{"thread":{"id":"53267","subject":"[PATCH 0/3] commit-graph: write non-split graphs as read-only","startedAt":"2020-04-20T22:51:03Z","lastAt":"2020-05-01T05:59:49Z","messageCount":36,"participants":["Taylor Blau","Junio C Hamano","Jeff King","Derrick Stolee"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"395776","messageId":"cover.1587422630.git.me@ttaylorr.com","threadId":"53267","inReplyTo":null,"subject":"[PATCH 0/3] commit-graph: write non-split graphs as read-only","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-04-20T22:50:55Z","receivedAt":"2020-04-20T22:51:03Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"Hi,\n\nA colleague of mine pointed out they had occasionally observed that some\nlayers of a commit-graph chain appear to be have both read and write\npermission bits for the user, while others are marked as read-only.\n\nAfter some investigation, I realized that this only happens to the first\nlayer in a commit-graph-chain, and occurs when we move a non-split graph\ninto a chain as the new base. Since write_commit_graph() just renames in\nthis case, we carry over the 0644 permissions that we create non-split\ngraphs with, even though split graphs are created to be read-only.\n\nInitially, I resolved this by 'chmod(graph, 0444)'-ing the graph after\nrenaming it into place, but it ends up being much cleaner if we\nintroduce an additional parameter in 'hold_lock_file_for_update' for a\nmode.\n\nThe first two patches set this up, and the third patch uses it in\ncommit-graph.c, and corrects some test fallout. Eventually, we may want\nto take another look at all of this and always create lockfiles with\npermission 0444, but that change is much larger than this one and could\ninstead be done over time.\n\nThanks,\nTaylor\n\nTaylor Blau (3):\n tempfile.c: introduce 'create_tempfile_mode'\n lockfile.c: introduce 'hold_lock_file_for_update_mode'\n commit-graph.c: write non-split graphs as read-only\n\n commit-graph.c | 3 ++-\n lockfile.c | 18 ++++++++++--------\n lockfile.h | 19 +++++++++++++++++--\n t/t5318-commit-graph.sh | 11 ++++++++++-\n t/t6600-test-reach.sh | 2 ++\n tempfile.c | 6 +++---\n tempfile.h | 7 ++++++-\n 7 files changed, 50 insertions(+), 16 deletions(-)\n\n--\n2.26.1.108.gadb95c98e4\n"},{"id":"395777","messageId":"aa86e8df403eef31295e6036f280995fa74cc3b4.1587422630.git.me@ttaylorr.com","threadId":"53267","inReplyTo":"cover.1587422630.git.me@ttaylorr.com","subject":"[PATCH 1/3] tempfile.c: introduce 'create_tempfile_mode'","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-04-20T22:51:03Z","receivedAt":"2020-04-20T22:51:06Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"In the next patch, 'hold_lock_file_for_update' will gain an additional\n'mode' parameter to specify permissions for the associated temporary\nfile.\n\nSince the lockfile.c machinery uses 'create_tempfile' which always\ncreates a temporary file with global read-write permissions, introduce a\nvariant here that allows specifying the mode.\n\nArguably, all temporary files should have permission 0444, since they\nare likely to be renamed into place and then not written to again. This\nis a much larger change than we may want to take on in this otherwise\nsmall patch, so for the time being, make 'create_tempfile' behave as it\nhas always done by inlining it to 'create_tempfile_mode' with mode set\nto '0666'.\n\nSigned-off-by: Taylor Blau \n---\n tempfile.c | 6 +++---\n tempfile.h | 7 ++++++-\n 2 files changed, 9 insertions(+), 4 deletions(-)\n\ndiff --git a/tempfile.c b/tempfile.c\nindex d43ad8c191..94aa18f3f7 100644\n--- a/tempfile.c\n+++ b/tempfile.c\n@@ -130,17 +130,17 @@ static void deactivate_tempfile(struct tempfile *tempfile)\n }\n \n /* Make sure errno contains a meaningful value on error */\n-struct tempfile *create_tempfile(const char *path)\n+struct tempfile *create_tempfile_mode(const char *path, int mode)\n {\n \tstruct tempfile *tempfile = new_tempfile();\n \n \tstrbuf_add_absolute_path(&tempfile->filename, path);\n \ttempfile->fd = open(tempfile->filename.buf,\n-\t\t\t O_RDWR | O_CREAT | O_EXCL | O_CLOEXEC, 0666);\n+\t\t\t O_RDWR | O_CREAT | O_EXCL | O_CLOEXEC, mode);\n \tif (O_CLOEXEC && tempfile->fd < 0 && errno == EINVAL)\n \t\t/* Try again w/o O_CLOEXEC: the kernel might not support it */\n \t\ttempfile->fd = open(tempfile->filename.buf,\n-\t\t\t\t O_RDWR | O_CREAT | O_EXCL, 0666);\n+\t\t\t\t O_RDWR | O_CREAT | O_EXCL, mode);\n \tif (tempfile->fd < 0) {\n \t\tdeactivate_tempfile(tempfile);\n \t\treturn NULL;\ndiff --git a/tempfile.h b/tempfile.h\nindex cddda0a33c..b5760d4581 100644\n--- a/tempfile.h\n+++ b/tempfile.h\n@@ -89,7 +89,12 @@ struct tempfile {\n * a tempfile (whose \"fd\" member can be used for writing to it), or\n * NULL on error. It is an error if a file already exists at that path.\n */\n-struct tempfile *create_tempfile(const char *path);\n+struct tempfile *create_tempfile_mode(const char *path, int mode);\n+\n+static inline struct tempfile *create_tempfile(const char *path)\n+{\n+\treturn create_tempfile_mode(path, 0666);\n+}\n \n /*\n * Register an existing file as a tempfile, meaning that it will be\n-- \n2.26.1.108.gadb95c98e4\n\n"},{"id":"395778","messageId":"dad37d42334954d40f308a4fc3189a925f90df66.1587422630.git.me@ttaylorr.com","threadId":"53267","inReplyTo":"cover.1587422630.git.me@ttaylorr.com","subject":"[PATCH 2/3] lockfile.c: introduce 'hold_lock_file_for_update_mode'","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-04-20T22:51:06Z","receivedAt":"2020-04-20T22:51:10Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"We use 'hold_lock_file_for_update' (and the '_timeout') variant to\nacquire a lock when updating references, the commit-graph file, and so\non.\n\nIn particular, the commit-graph machinery uses this to acquire a\ntemporary file that is used to write a non-split commit-graph. In a\nsubsequent commit, an issue in the commit-graph machinery produces\ngraph files that have a different permission based on whether or not\nthey are part of a multi-layer graph will be addressed.\n\nTo do so, the commit-graph machinery will need a version of\n'hold_lock_file_for_update' that takes the permission bits from the\ncaller.\n\nIntroduce such a function in this patch for both the\n'hold_lock_file_for_update' and 'hold_lock_file_for_update_timeout'\nfunctions, and leave the existing functions alone by inlining their\ndefinitions in terms of the new mode variants.\n\nNote that even though the commit-graph machinery only calls\n'hold_lock_file_for_update', that this is defined in terms of\n'hold_lock_file_for_update_timeout', and so both need an additional mode\nparameter here.\n\nSigned-off-by: Taylor Blau \n---\n lockfile.c | 18 ++++++++++--------\n lockfile.h | 19 +++++++++++++++++--\n 2 files changed, 27 insertions(+), 10 deletions(-)\n\ndiff --git a/lockfile.c b/lockfile.c\nindex 8e8ab4f29f..cc9a4b8428 100644\n--- a/lockfile.c\n+++ b/lockfile.c\n@@ -70,7 +70,8 @@ static void resolve_symlink(struct strbuf *path)\n }\n \n /* Make sure errno contains a meaningful value on error */\n-static int lock_file(struct lock_file *lk, const char *path, int flags)\n+static int lock_file(struct lock_file *lk, const char *path, int flags,\n+\t\t int mode)\n {\n \tstruct strbuf filename = STRBUF_INIT;\n \n@@ -79,7 +80,7 @@ static int lock_file(struct lock_file *lk, const char *path, int flags)\n \t\tresolve_symlink(&filename);\n \n \tstrbuf_addstr(&filename, LOCK_SUFFIX);\n-\tlk->tempfile = create_tempfile(filename.buf);\n+\tlk->tempfile = create_tempfile_mode(filename.buf, mode);\n \tstrbuf_release(&filename);\n \treturn lk->tempfile ? lk->tempfile->fd : -1;\n }\n@@ -99,7 +100,7 @@ static int lock_file(struct lock_file *lk, const char *path, int flags)\n * exactly once. If timeout_ms is -1, try indefinitely.\n */\n static int lock_file_timeout(struct lock_file *lk, const char *path,\n-\t\t\t int flags, long timeout_ms)\n+\t\t\t int flags, long timeout_ms, int mode)\n {\n \tint n = 1;\n \tint multiplier = 1;\n@@ -107,7 +108,7 @@ static int lock_file_timeout(struct lock_file *lk, const char *path,\n \tstatic int random_initialized = 0;\n \n \tif (timeout_ms == 0)\n-\t\treturn lock_file(lk, path, flags);\n+\t\treturn lock_file(lk, path, flags, mode);\n \n \tif (!random_initialized) {\n \t\tsrand((unsigned int)getpid());\n@@ -121,7 +122,7 @@ static int lock_file_timeout(struct lock_file *lk, const char *path,\n \t\tlong backoff_ms, wait_ms;\n \t\tint fd;\n \n-\t\tfd = lock_file(lk, path, flags);\n+\t\tfd = lock_file(lk, path, flags, mode);\n \n \t\tif (fd >= 0)\n \t\t\treturn fd; /* success */\n@@ -169,10 +170,11 @@ NORETURN void unable_to_lock_die(const char *path, int err)\n }\n \n /* This should return a meaningful errno on failure */\n-int hold_lock_file_for_update_timeout(struct lock_file *lk, const char *path,\n-\t\t\t\t int flags, long timeout_ms)\n+int hold_lock_file_for_update_timeout_mode(struct lock_file *lk,\n+\t\t\t\t\t const char *path, int flags,\n+\t\t\t\t\t long timeout_ms, int mode)\n {\n-\tint fd = lock_file_timeout(lk, path, flags, timeout_ms);\n+\tint fd = lock_file_timeout(lk, path, flags, timeout_ms, mode);\n \tif (fd < 0) {\n \t\tif (flags & LOCK_DIE_ON_ERROR)\n \t\t\tunable_to_lock_die(path, errno);\ndiff --git a/lockfile.h b/lockfile.h\nindex 9843053ce8..12f25279b5 100644\n--- a/lockfile.h\n+++ b/lockfile.h\n@@ -159,9 +159,17 @@ struct lock_file {\n * timeout_ms is -1, retry indefinitely. The flags argument and error\n * handling are described above.\n */\n-int hold_lock_file_for_update_timeout(\n+int hold_lock_file_for_update_timeout_mode(\n \t\tstruct lock_file *lk, const char *path,\n-\t\tint flags, long timeout_ms);\n+\t\tint flags, long timeout_ms, int mode);\n+\n+static inline int hold_lock_file_for_update_timeout(\n+\t\tstruct lock_file *lk, const char *path,\n+\t\tint flags, long timeout_ms)\n+{\n+\treturn hold_lock_file_for_update_timeout_mode(lk, path, flags,\n+\t\t\t\t\t\t timeout_ms, 0666);\n+}\n \n /*\n * Attempt to create a lockfile for the file at `path` and return a\n@@ -175,6 +183,13 @@ static inline int hold_lock_file_for_update(\n \treturn hold_lock_file_for_update_timeout(lk, path, flags, 0);\n }\n \n+static inline int hold_lock_file_for_update_mode(\n+\t\tstruct lock_file *lk, const char *path,\n+\t\tint flags, int mode)\n+{\n+\treturn hold_lock_file_for_update_timeout_mode(lk, path, flags, 0, mode);\n+}\n+\n /*\n * Return a nonzero value iff `lk` is currently locked.\n */\n-- \n2.26.1.108.gadb95c98e4\n\n"},{"id":"395779","messageId":"622fd92cee7ccf035c348f64599cdc487b34717b.1587422630.git.me@ttaylorr.com","threadId":"53267","inReplyTo":"cover.1587422630.git.me@ttaylorr.com","subject":"[PATCH 3/3] commit-graph.c: write non-split graphs as read-only","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-04-20T22:51:10Z","receivedAt":"2020-04-20T22:51:13Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"In the previous commit, Git learned 'hold_lock_file_for_update_mode' to\nallow the caller to specify the permission bits used when acquiring a\ntemporary file.\n\nUse this in the commit-graph machinery for writing a non-split graph to\nacquire an opened temporary file with permissions read-only permissions\nto match the split behavior. (In the split case, Git uses\n'git_mkstemp_mode' for each of the commit-graph layers with permission\nbits '0444').\n\nOne can notice this discrepancy when moving a non-split graph to be part\nof a new chain. This causes a commit-graph chain where all layers have\nread-only permission bits, except for the base layer, which is writable\nfor the current user.\n\nResolve this discrepancy by using the new\n'hold_lock_file_for_update_mode' and passing the desired permission\nbits.\n\nDoing so causes some test fallout in t5318 and t6600. In t5318, this\noccurs in tests that corrupt a commit-graph file by writing into it. For\nthese, 'chmod u+w'-ing the file beforehand resolves the issue. The\nadditional spot in 'corrupt_graph_verify' is necessary because of the\nextra 'git commit-graph write' beforehand (which *does* rewrite the\ncommit-graph file). In t6600, this is caused by copying a read-only\ncommit-graph file into place and then trying to replace it. For these,\nmake these files writable.\n\nSigned-off-by: Taylor Blau \n---\n commit-graph.c | 3 ++-\n t/t5318-commit-graph.sh | 11 ++++++++++-\n t/t6600-test-reach.sh | 2 ++\n 3 files changed, 14 insertions(+), 2 deletions(-)\n\ndiff --git a/commit-graph.c b/commit-graph.c\nindex f013a84e29..5b5047a7dd 100644\n--- a/commit-graph.c\n+++ b/commit-graph.c\n@@ -1388,7 +1388,8 @@ static int write_commit_graph_file(struct write_commit_graph_context *ctx)\n \n \t\tf = hashfd(fd, ctx->graph_name);\n \t} else {\n-\t\thold_lock_file_for_update(&lk, ctx->graph_name, LOCK_DIE_ON_ERROR);\n+\t\thold_lock_file_for_update_mode(&lk, ctx->graph_name,\n+\t\t\t\t\t LOCK_DIE_ON_ERROR, 0444);\n \t\tfd = lk.tempfile->fd;\n \t\tf = hashfd(lk.tempfile->fd, lk.tempfile->filename.buf);\n \t}\ndiff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh\nindex 9bf920ae17..fb0aae61c3 100755\n--- a/t/t5318-commit-graph.sh\n+++ b/t/t5318-commit-graph.sh\n@@ -96,6 +96,13 @@ test_expect_success 'write graph' '\n \tgraph_read_expect \"3\"\n '\n \n+test_expect_success POSIXPERM 'write graph has correct permissions' '\n+\ttest_path_is_file $objdir/info/commit-graph &&\n+\techo \"-r--r--r--\" >expect &&\n+\ttest_modebits $objdir/info/commit-graph >actual &&\n+\ttest_cmp expect actual\n+'\n+\n graph_git_behavior 'graph exists' full commits/3 commits/1\n \n test_expect_success 'Add more commits' '\n@@ -421,7 +428,8 @@ GRAPH_BYTE_FOOTER=$(($GRAPH_OCTOPUS_DATA_OFFSET + 4 * $NUM_OCTOPUS_EDGES))\n corrupt_graph_setup() {\n \tcd \"$TRASH_DIRECTORY/full\" &&\n \ttest_when_finished mv commit-graph-backup $objdir/info/commit-graph &&\n-\tcp $objdir/info/commit-graph commit-graph-backup\n+\tcp $objdir/info/commit-graph commit-graph-backup &&\n+\tchmod u+w $objdir/info/commit-graph\n }\n \n corrupt_graph_verify() {\n@@ -435,6 +443,7 @@ corrupt_graph_verify() {\n \tfi &&\n \tgit status --short &&\n \tGIT_TEST_COMMIT_GRAPH_DIE_ON_LOAD=true git commit-graph write &&\n+\tchmod u+w $objdir/info/commit-graph &&\n \tgit commit-graph verify\n }\n \ndiff --git a/t/t6600-test-reach.sh b/t/t6600-test-reach.sh\nindex b24d850036..475564bee7 100755\n--- a/t/t6600-test-reach.sh\n+++ b/t/t6600-test-reach.sh\n@@ -51,8 +51,10 @@ test_expect_success 'setup' '\n \tdone &&\n \tgit commit-graph write --reachable &&\n \tmv .git/objects/info/commit-graph commit-graph-full &&\n+\tchmod u+w commit-graph-full &&\n \tgit show-ref -s commit-5-5 | git commit-graph write --stdin-commits &&\n \tmv .git/objects/info/commit-graph commit-graph-half &&\n+\tchmod u+w commit-graph-half &&\n \tgit config core.commitGraph true\n '\n \n-- \n2.26.1.108.gadb95c98e4\n"},{"id":"395784","messageId":"xmqq5zdtrbby.fsf@gitster.c.googlers.com","threadId":"53267","inReplyTo":"cover.1587422630.git.me@ttaylorr.com","subject":"Re: [PATCH 0/3] commit-graph: write non-split graphs as read-only","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-04-20T23:23:13Z","receivedAt":"2020-04-20T23:23:19Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Taylor Blau writes:\n\n> The first two patches set this up, and the third patch uses it in\n> commit-graph.c, and corrects some test fallout. Eventually, we may want\n> to take another look at all of this and always create lockfiles with\n> permission 0444, but that change is much larger than this one and could\n> instead be done over time.\n\nSo,... is the problem that this did not follow the common pattern of\ncreating (while honoring umask) and then call adjust_shared_perm(),\nwhich I thought was the common pattern for files in $GIT_DIR/?\n"},{"id":"395785","messageId":"xmqq1rohrayh.fsf@gitster.c.googlers.com","threadId":"53267","inReplyTo":"aa86e8df403eef31295e6036f280995fa74cc3b4.1587422630.git.me@ttaylorr.com","subject":"Re: [PATCH 1/3] tempfile.c: introduce 'create_tempfile_mode'","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-04-20T23:31:18Z","receivedAt":"2020-04-20T23:31:25Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Taylor Blau writes:\n\n> Arguably, all temporary files should have permission 0444, since they\n> are likely to be renamed into place and then not written to again.\n\nIf the repository is not shared, it is more than likely that these\nfiles ought to have permission narrower than that. As you'd be\nconcluding any file that you created and written to in $GIT_DIR/\nwith adjust_shared_perm(), I do not think it is a good idea to allow\ncallers to pass an arbitrary mode and let them expect the mode would\nstick. Shoudln't we instead be passing \"int read_only\" and depending\non the boolean value, choosing between 0444 and 0666 in the function?\n\n> Signed-off-by: Taylor Blau \n> ---\n> tempfile.c | 6 +++---\n> tempfile.h | 7 ++++++-\n> 2 files changed, 9 insertions(+), 4 deletions(-)\n>\n> diff --git a/tempfile.c b/tempfile.c\n> index d43ad8c191..94aa18f3f7 100644\n> --- a/tempfile.c\n> +++ b/tempfile.c\n> @@ -130,17 +130,17 @@ static void deactivate_tempfile(struct tempfile *tempfile)\n> }\n> \n> /* Make sure errno contains a meaningful value on error */\n> -struct tempfile *create_tempfile(const char *path)\n> +struct tempfile *create_tempfile_mode(const char *path, int mode)\n> {\n> \tstruct tempfile *tempfile = new_tempfile();\n> \n> \tstrbuf_add_absolute_path(&tempfile->filename, path);\n> \ttempfile->fd = open(tempfile->filename.buf,\n> -\t\t\t O_RDWR | O_CREAT | O_EXCL | O_CLOEXEC, 0666);\n> +\t\t\t O_RDWR | O_CREAT | O_EXCL | O_CLOEXEC, mode);\n> \tif (O_CLOEXEC && tempfile->fd < 0 && errno == EINVAL)\n> \t\t/* Try again w/o O_CLOEXEC: the kernel might not support it */\n> \t\ttempfile->fd = open(tempfile->filename.buf,\n> -\t\t\t\t O_RDWR | O_CREAT | O_EXCL, 0666);\n> +\t\t\t\t O_RDWR | O_CREAT | O_EXCL, mode);\n> \tif (tempfile->fd < 0) {\n> \t\tdeactivate_tempfile(tempfile);\n> \t\treturn NULL;\n> diff --git a/tempfile.h b/tempfile.h\n> index cddda0a33c..b5760d4581 100644\n> --- a/tempfile.h\n> +++ b/tempfile.h\n> @@ -89,7 +89,12 @@ struct tempfile {\n> * a tempfile (whose \"fd\" member can be used for writing to it), or\n> * NULL on error. It is an error if a file already exists at that path.\n> */\n> -struct tempfile *create_tempfile(const char *path);\n> +struct tempfile *create_tempfile_mode(const char *path, int mode);\n> +\n> +static inline struct tempfile *create_tempfile(const char *path)\n> +{\n> +\treturn create_tempfile_mode(path, 0666);\n> +}\n> \n> /*\n> * Register an existing file as a tempfile, meaning that it will be\n"},{"id":"395786","messageId":"20200420233907.GA97996@syl.local","threadId":"53267","inReplyTo":"xmqq5zdtrbby.fsf@gitster.c.googlers.com","subject":"Re: [PATCH 0/3] commit-graph: write non-split graphs as read-only","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-04-20T23:39:07Z","receivedAt":"2020-04-20T23:39:12Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Mon, Apr 20, 2020 at 04:23:13PM -0700, Junio C Hamano wrote:\n> Taylor Blau writes:\n>\n> > The first two patches set this up, and the third patch uses it in\n> > commit-graph.c, and corrects some test fallout. Eventually, we may want\n> > to take another look at all of this and always create lockfiles with\n> > permission 0444, but that change is much larger than this one and could\n> > instead be done over time.\n>\n> So,... is the problem that this did not follow the common pattern of\n> creating (while honoring umask) and then call adjust_shared_perm(),\n> which I thought was the common pattern for files in $GIT_DIR/?\n\nWe do call adjust_shared_perm() from (based on what's currently in\nmaster) 'write_commit_graph_file' -> 'hold_lockfile_for_update' ->\n'lock_file_timeout' -> 'lock_file' -> 'create_tempfile'.\n\nBut do we want commit-graphs to be respecting 'core.sharedRepository'\nhere? Either we:\n\n * do want to respect core.sharedRepository, in which case the\n current behavior of always setting split-graph permissions to '0444'\n is wrong, or\n\n * we do not want to respect core.sharedRepository, in which case these\n patches are doing what we want by setting all commit-graph files to\n have read-only permissions.\n\nMy hunch is that we do not want to abide by core.sharedRepository here\nfor the same reason that, say, loose objects are read-only. What do you\nthink?\n\n\nThanks,\nTaylor\n"},{"id":"395801","messageId":"xmqqlfmpprhq.fsf@gitster.c.googlers.com","threadId":"53267","inReplyTo":"20200420233907.GA97996@syl.local","subject":"Re: [PATCH 0/3] commit-graph: write non-split graphs as read-only","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-04-21T01:17:05Z","receivedAt":"2020-04-21T01:17:10Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Taylor Blau writes:\n\n> * do want to respect core.sharedRepository, in which case the\n> current behavior of always setting split-graph permissions to '0444'\n> is wrong, or\n\nYes, I would say \"always 0444\" is wrong.\n\n> * we do not want to respect core.sharedRepository, in which case these\n> patches are doing what we want by setting all commit-graph files to\n> have read-only permissions.\n>\n> My hunch is that we do not want to abide by core.sharedRepository here\n> for the same reason that, say, loose objects are read-only. What do you\n> think?\n\nI thought that adjusting perm for sharedRepository is orthogonal to\nthe read-only vs read-write. If a file ought to be writable by the\nowner, we would make it group writable in a group shared repository.\nIf a file is readable by the owner, we make sure it is readable by\ngroup in such a repository (and we do not want to flip write bit\non). That happens by calling path.c::calc_shared_perm().\n\n"},{"id":"395814","messageId":"20200421070135.GA2906244@coredump.intra.peff.net","threadId":"53267","inReplyTo":"xmqqlfmpprhq.fsf@gitster.c.googlers.com","subject":"Re: [PATCH 0/3] commit-graph: write non-split graphs as read-only","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-04-21T07:01:35Z","receivedAt":"2020-04-21T07:01:37Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Apr 20, 2020 at 06:17:05PM -0700, Junio C Hamano wrote:\n\n> Taylor Blau writes:\n> \n> > * do want to respect core.sharedRepository, in which case the\n> > current behavior of always setting split-graph permissions to '0444'\n> > is wrong, or\n> \n> Yes, I would say \"always 0444\" is wrong.\n\nI'm not sure. That's what we do for loose objects, packs, etc. The mode\nwe feed to git_mkstemp_mode(), etc, is not the true mode we expect to\nend up with. We know that it will be modified by the umask, and then\nperhaps by adjust_shared_perm().\n\nIf you are arguing that there are only two interesting modes: 0444 and\n0666, and those could be represented by a read-only/read-write enum, I'd\nagree with that. But the rest of the code already spells those as 0444\nand 0666, and I don't see that as a big problem (in fact, there's a bit\nof an advantage because the same constants work at both high and low\nlevels of the call stack, so you don't have to decide where to put the\ntranslation).\n\n> > * we do not want to respect core.sharedRepository, in which case these\n> > patches are doing what we want by setting all commit-graph files to\n> > have read-only permissions.\n> >\n> > My hunch is that we do not want to abide by core.sharedRepository here\n> > for the same reason that, say, loose objects are read-only. What do you\n> > think?\n> \n> I thought that adjusting perm for sharedRepository is orthogonal to\n> the read-only vs read-write. If a file ought to be writable by the\n> owner, we would make it group writable in a group shared repository.\n> If a file is readable by the owner, we make sure it is readable by\n> group in such a repository (and we do not want to flip write bit\n> on). That happens by calling path.c::calc_shared_perm().\n\nRight. I think adjust_shared_perm() should already be doing what we\nwant, and we should continue to call it. But it should not be\nresponsible for this \"read-only versus read-write\" decision. That\nhappens much earlier, and it adjusts as appropriate.\n\n-Peff\n"},{"id":"395844","messageId":"xmqqk128oebd.fsf@gitster.c.googlers.com","threadId":"53267","inReplyTo":"20200421070135.GA2906244@coredump.intra.peff.net","subject":"Re: [PATCH 0/3] commit-graph: write non-split graphs as read-only","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-04-21T18:59:18Z","receivedAt":"2020-04-21T18:59:23Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n>> Yes, I would say \"always 0444\" is wrong.\n>\n> I'm not sure. That's what we do for loose objects, packs, etc. The mode\n> we feed to git_mkstemp_mode(), etc, is not the true mode we expect to\n> end up with. We know that it will be modified by the umask, and then\n> perhaps by adjust_shared_perm().\n>\n> If you are arguing that there are only two interesting modes: 0444 and\n> 0666, and those could be represented by a read-only/read-write enum, I'd\n> agree with that.\n\nYup, that is what I meant. I am aware that these 0444/0666 are\nlimited with umask at open(O_CREAT) time, and then we later call\nadjust_shared_perm(). I thought Taylor meant to always make it\nreadable by everybody, which was the only point I was objecting to.\n\n> Right. I think adjust_shared_perm() should already be doing what we\n> want, and we should continue to call it. But it should not be\n> responsible for this \"read-only versus read-write\" decision. That\n> happens much earlier, and it adjusts as appropriate.\n\nYes.\n"},{"id":"396343","messageId":"cover.1588004647.git.me@ttaylorr.com","threadId":"53267","inReplyTo":"cover.1587422630.git.me@ttaylorr.com","subject":"[PATCH v2 0/4] commit-graph: write non-split graphs as read-only","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-04-27T16:27:49Z","receivedAt":"2020-04-27T16:27:56Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"Hi,\n\nHere's an updated version of my series to resolve a difference between\nthe permissions that split- and non-split commit-graphs are written\nwith.\n\nNot much has changed since last time, since the main discussion revolved\naround differences between what mode the user passes and how that\ninteracts with 'adjust_shared_perm' as well as why split-graphs are\nalways read-only for everybody.\n\nPatches 1-3 remain mostly the same, with only some additional\ndocumentation in the patches and headers about the interaction between\nthe 'mode' parameter and the umask and 'core.sharedRepository'. The\nfourth patch is new, and makes sure that split-graphs are also\nrespecting 'core.sharedRepository' (and never have permissions above\n0444).\n\nThe end-state of this is that both split- and non-split graphs have at\nmost permission 0444, and both respect core.sharedRepository.\n\nThanks in advance for another look.\n\n\nTaylor Blau (4):\n tempfile.c: introduce 'create_tempfile_mode'\n lockfile.c: introduce 'hold_lock_file_for_update_mode'\n commit-graph.c: write non-split graphs as read-only\n commit-graph.c: ensure graph layers respect core.sharedRepository\n\n commit-graph.c | 9 ++++++++-\n lockfile.c | 18 ++++++++++--------\n lockfile.h | 32 ++++++++++++++++++++++++++++----\n t/t5318-commit-graph.sh | 11 ++++++++++-\n t/t5324-split-commit-graph.sh | 18 ++++++++++++++++++\n t/t6600-test-reach.sh | 2 ++\n tempfile.c | 6 +++---\n tempfile.h | 10 +++++++++-\n 8 files changed, 88 insertions(+), 18 deletions(-)\n\nRange-diff against v1:\n1: aa86e8df40 ! 1: 03c975b0bd tempfile.c: introduce 'create_tempfile_mode'\n @@ Commit message\n creates a temporary file with global read-write permissions, introduce a\n variant here that allows specifying the mode.\n\n + Note that the mode given to 'create_tempfile_mode' is not guaranteed to\n + be written to disk, since it is subject to both the umask and\n + 'core.sharedRepository'.\n +\n Arguably, all temporary files should have permission 0444, since they\n are likely to be renamed into place and then not written to again. This\n is a much larger change than we may want to take on in this otherwise\n @@ tempfile.c: static void deactivate_tempfile(struct tempfile *tempfile)\n\n ## tempfile.h ##\n @@ tempfile.h: struct tempfile {\n + * Attempt to create a temporary file at the specified `path`. Return\n * a tempfile (whose \"fd\" member can be used for writing to it), or\n * NULL on error. It is an error if a file already exists at that path.\n ++ * Note that `mode` will be further modified by the umask, and possibly\n ++ * `core.sharedRepository`, so it is not guaranteed to have the given\n ++ * mode.\n */\n -struct tempfile *create_tempfile(const char *path);\n +struct tempfile *create_tempfile_mode(const char *path, int mode);\n2: dad37d4233 ! 2: c1c84552bc lockfile.c: introduce 'hold_lock_file_for_update_mode'\n @@ Commit message\n functions, and leave the existing functions alone by inlining their\n definitions in terms of the new mode variants.\n\n - Note that even though the commit-graph machinery only calls\n + Note that, like in the previous commit, 'hold_lock_file_for_update_mode'\n + is not guarenteed to set the given mode, since it may be modified by\n + both the umask and 'core.sharedRepository'.\n +\n + Note also that even though the commit-graph machinery only calls\n 'hold_lock_file_for_update', that this is defined in terms of\n 'hold_lock_file_for_update_timeout', and so both need an additional mode\n parameter here.\n @@ lockfile.c: NORETURN void unable_to_lock_die(const char *path, int err)\n \t\t\tunable_to_lock_die(path, errno);\n\n ## lockfile.h ##\n +@@\n + * functions. In particular, the state diagram and the cleanup\n + * machinery are all implemented in the tempfile module.\n + *\n ++ * Permission bits\n ++ * ---------------\n ++ *\n ++ * If you call either `hold_lock_file_for_update_mode` or\n ++ * `hold_lock_file_for_update_timeout_mode`, you can specify a suggested\n ++ * mode for the underlying temporary file. Note that the file isn't\n ++ * guaranteed to have this exact mode, since it may be limited by either\n ++ * the umask, 'core.sharedRepository', or both. See `adjust_shared_perm`\n ++ * for more.\n + *\n + * Error handling\n + * --------------\n @@ lockfile.h: struct lock_file {\n - * timeout_ms is -1, retry indefinitely. The flags argument and error\n - * handling are described above.\n + * file descriptor for writing to it, or -1 on error. If the file is\n + * currently locked, retry with quadratic backoff for at least\n + * timeout_ms milliseconds. If timeout_ms is 0, try exactly once; if\n +- * timeout_ms is -1, retry indefinitely. The flags argument and error\n +- * handling are described above.\n ++ * timeout_ms is -1, retry indefinitely. The flags argument, error\n ++ * handling, and mode are described above.\n */\n -int hold_lock_file_for_update_timeout(\n +int hold_lock_file_for_update_timeout_mode(\n3: 622fd92cee ! 3: 86cf29ce9c commit-graph.c: write non-split graphs as read-only\n @@ Commit message\n commit-graph.c: write non-split graphs as read-only\n\n In the previous commit, Git learned 'hold_lock_file_for_update_mode' to\n - allow the caller to specify the permission bits used when acquiring a\n - temporary file.\n + allow the caller to specify the permission bits (prior to further\n + adjustment by the umask and shared repository permissions) used when\n + acquiring a temporary file.\n\n Use this in the commit-graph machinery for writing a non-split graph to\n acquire an opened temporary file with permissions read-only permissions\n to match the split behavior. (In the split case, Git uses\n - 'git_mkstemp_mode' for each of the commit-graph layers with permission\n + git_mkstemp_mode' for each of the commit-graph layers with permission\n bits '0444').\n\n One can notice this discrepancy when moving a non-split graph to be part\n-: ---------- > 4: f83437f130 commit-graph.c: ensure graph layers respect core.sharedRepository\n--\n2.26.0.113.ge9739cdccc\n"},{"id":"396344","messageId":"03c975b0bd0cd3dd5d693187f12f45fd7565016c.1588004647.git.me@ttaylorr.com","threadId":"53267","inReplyTo":"cover.1588004647.git.me@ttaylorr.com","subject":"[PATCH v2 1/4] tempfile.c: introduce 'create_tempfile_mode'","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-04-27T16:27:54Z","receivedAt":"2020-04-27T16:28:00Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"In the next patch, 'hold_lock_file_for_update' will gain an additional\n'mode' parameter to specify permissions for the associated temporary\nfile.\n\nSince the lockfile.c machinery uses 'create_tempfile' which always\ncreates a temporary file with global read-write permissions, introduce a\nvariant here that allows specifying the mode.\n\nNote that the mode given to 'create_tempfile_mode' is not guaranteed to\nbe written to disk, since it is subject to both the umask and\n'core.sharedRepository'.\n\nArguably, all temporary files should have permission 0444, since they\nare likely to be renamed into place and then not written to again. This\nis a much larger change than we may want to take on in this otherwise\nsmall patch, so for the time being, make 'create_tempfile' behave as it\nhas always done by inlining it to 'create_tempfile_mode' with mode set\nto '0666'.\n\nSigned-off-by: Taylor Blau \n---\n tempfile.c | 6 +++---\n tempfile.h | 10 +++++++++-\n 2 files changed, 12 insertions(+), 4 deletions(-)\n\ndiff --git a/tempfile.c b/tempfile.c\nindex d43ad8c191..94aa18f3f7 100644\n--- a/tempfile.c\n+++ b/tempfile.c\n@@ -130,17 +130,17 @@ static void deactivate_tempfile(struct tempfile *tempfile)\n }\n \n /* Make sure errno contains a meaningful value on error */\n-struct tempfile *create_tempfile(const char *path)\n+struct tempfile *create_tempfile_mode(const char *path, int mode)\n {\n \tstruct tempfile *tempfile = new_tempfile();\n \n \tstrbuf_add_absolute_path(&tempfile->filename, path);\n \ttempfile->fd = open(tempfile->filename.buf,\n-\t\t\t O_RDWR | O_CREAT | O_EXCL | O_CLOEXEC, 0666);\n+\t\t\t O_RDWR | O_CREAT | O_EXCL | O_CLOEXEC, mode);\n \tif (O_CLOEXEC && tempfile->fd < 0 && errno == EINVAL)\n \t\t/* Try again w/o O_CLOEXEC: the kernel might not support it */\n \t\ttempfile->fd = open(tempfile->filename.buf,\n-\t\t\t\t O_RDWR | O_CREAT | O_EXCL, 0666);\n+\t\t\t\t O_RDWR | O_CREAT | O_EXCL, mode);\n \tif (tempfile->fd < 0) {\n \t\tdeactivate_tempfile(tempfile);\n \t\treturn NULL;\ndiff --git a/tempfile.h b/tempfile.h\nindex cddda0a33c..4de3bc77d2 100644\n--- a/tempfile.h\n+++ b/tempfile.h\n@@ -88,8 +88,16 @@ struct tempfile {\n * Attempt to create a temporary file at the specified `path`. Return\n * a tempfile (whose \"fd\" member can be used for writing to it), or\n * NULL on error. It is an error if a file already exists at that path.\n+ * Note that `mode` will be further modified by the umask, and possibly\n+ * `core.sharedRepository`, so it is not guaranteed to have the given\n+ * mode.\n */\n-struct tempfile *create_tempfile(const char *path);\n+struct tempfile *create_tempfile_mode(const char *path, int mode);\n+\n+static inline struct tempfile *create_tempfile(const char *path)\n+{\n+\treturn create_tempfile_mode(path, 0666);\n+}\n \n /*\n * Register an existing file as a tempfile, meaning that it will be\n-- \n2.26.0.113.ge9739cdccc\n\n"},{"id":"396345","messageId":"c1c84552bc69218299ce182388b6a888b3c71056.1588004647.git.me@ttaylorr.com","threadId":"53267","inReplyTo":"cover.1588004647.git.me@ttaylorr.com","subject":"[PATCH v2 2/4] lockfile.c: introduce 'hold_lock_file_for_update_mode'","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-04-27T16:27:58Z","receivedAt":"2020-04-27T16:28:03Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"We use 'hold_lock_file_for_update' (and the '_timeout') variant to\nacquire a lock when updating references, the commit-graph file, and so\non.\n\nIn particular, the commit-graph machinery uses this to acquire a\ntemporary file that is used to write a non-split commit-graph. In a\nsubsequent commit, an issue in the commit-graph machinery produces\ngraph files that have a different permission based on whether or not\nthey are part of a multi-layer graph will be addressed.\n\nTo do so, the commit-graph machinery will need a version of\n'hold_lock_file_for_update' that takes the permission bits from the\ncaller.\n\nIntroduce such a function in this patch for both the\n'hold_lock_file_for_update' and 'hold_lock_file_for_update_timeout'\nfunctions, and leave the existing functions alone by inlining their\ndefinitions in terms of the new mode variants.\n\nNote that, like in the previous commit, 'hold_lock_file_for_update_mode'\nis not guarenteed to set the given mode, since it may be modified by\nboth the umask and 'core.sharedRepository'.\n\nNote also that even though the commit-graph machinery only calls\n'hold_lock_file_for_update', that this is defined in terms of\n'hold_lock_file_for_update_timeout', and so both need an additional mode\nparameter here.\n\nSigned-off-by: Taylor Blau \n---\n lockfile.c | 18 ++++++++++--------\n lockfile.h | 32 ++++++++++++++++++++++++++++----\n 2 files changed, 38 insertions(+), 12 deletions(-)\n\ndiff --git a/lockfile.c b/lockfile.c\nindex 8e8ab4f29f..cc9a4b8428 100644\n--- a/lockfile.c\n+++ b/lockfile.c\n@@ -70,7 +70,8 @@ static void resolve_symlink(struct strbuf *path)\n }\n \n /* Make sure errno contains a meaningful value on error */\n-static int lock_file(struct lock_file *lk, const char *path, int flags)\n+static int lock_file(struct lock_file *lk, const char *path, int flags,\n+\t\t int mode)\n {\n \tstruct strbuf filename = STRBUF_INIT;\n \n@@ -79,7 +80,7 @@ static int lock_file(struct lock_file *lk, const char *path, int flags)\n \t\tresolve_symlink(&filename);\n \n \tstrbuf_addstr(&filename, LOCK_SUFFIX);\n-\tlk->tempfile = create_tempfile(filename.buf);\n+\tlk->tempfile = create_tempfile_mode(filename.buf, mode);\n \tstrbuf_release(&filename);\n \treturn lk->tempfile ? lk->tempfile->fd : -1;\n }\n@@ -99,7 +100,7 @@ static int lock_file(struct lock_file *lk, const char *path, int flags)\n * exactly once. If timeout_ms is -1, try indefinitely.\n */\n static int lock_file_timeout(struct lock_file *lk, const char *path,\n-\t\t\t int flags, long timeout_ms)\n+\t\t\t int flags, long timeout_ms, int mode)\n {\n \tint n = 1;\n \tint multiplier = 1;\n@@ -107,7 +108,7 @@ static int lock_file_timeout(struct lock_file *lk, const char *path,\n \tstatic int random_initialized = 0;\n \n \tif (timeout_ms == 0)\n-\t\treturn lock_file(lk, path, flags);\n+\t\treturn lock_file(lk, path, flags, mode);\n \n \tif (!random_initialized) {\n \t\tsrand((unsigned int)getpid());\n@@ -121,7 +122,7 @@ static int lock_file_timeout(struct lock_file *lk, const char *path,\n \t\tlong backoff_ms, wait_ms;\n \t\tint fd;\n \n-\t\tfd = lock_file(lk, path, flags);\n+\t\tfd = lock_file(lk, path, flags, mode);\n \n \t\tif (fd >= 0)\n \t\t\treturn fd; /* success */\n@@ -169,10 +170,11 @@ NORETURN void unable_to_lock_die(const char *path, int err)\n }\n \n /* This should return a meaningful errno on failure */\n-int hold_lock_file_for_update_timeout(struct lock_file *lk, const char *path,\n-\t\t\t\t int flags, long timeout_ms)\n+int hold_lock_file_for_update_timeout_mode(struct lock_file *lk,\n+\t\t\t\t\t const char *path, int flags,\n+\t\t\t\t\t long timeout_ms, int mode)\n {\n-\tint fd = lock_file_timeout(lk, path, flags, timeout_ms);\n+\tint fd = lock_file_timeout(lk, path, flags, timeout_ms, mode);\n \tif (fd < 0) {\n \t\tif (flags & LOCK_DIE_ON_ERROR)\n \t\t\tunable_to_lock_die(path, errno);\ndiff --git a/lockfile.h b/lockfile.h\nindex 9843053ce8..db93e6ba73 100644\n--- a/lockfile.h\n+++ b/lockfile.h\n@@ -90,6 +90,15 @@\n * functions. In particular, the state diagram and the cleanup\n * machinery are all implemented in the tempfile module.\n *\n+ * Permission bits\n+ * ---------------\n+ *\n+ * If you call either `hold_lock_file_for_update_mode` or\n+ * `hold_lock_file_for_update_timeout_mode`, you can specify a suggested\n+ * mode for the underlying temporary file. Note that the file isn't\n+ * guaranteed to have this exact mode, since it may be limited by either\n+ * the umask, 'core.sharedRepository', or both. See `adjust_shared_perm`\n+ * for more.\n *\n * Error handling\n * --------------\n@@ -156,12 +165,20 @@ struct lock_file {\n * file descriptor for writing to it, or -1 on error. If the file is\n * currently locked, retry with quadratic backoff for at least\n * timeout_ms milliseconds. If timeout_ms is 0, try exactly once; if\n- * timeout_ms is -1, retry indefinitely. The flags argument and error\n- * handling are described above.\n+ * timeout_ms is -1, retry indefinitely. The flags argument, error\n+ * handling, and mode are described above.\n */\n-int hold_lock_file_for_update_timeout(\n+int hold_lock_file_for_update_timeout_mode(\n \t\tstruct lock_file *lk, const char *path,\n-\t\tint flags, long timeout_ms);\n+\t\tint flags, long timeout_ms, int mode);\n+\n+static inline int hold_lock_file_for_update_timeout(\n+\t\tstruct lock_file *lk, const char *path,\n+\t\tint flags, long timeout_ms)\n+{\n+\treturn hold_lock_file_for_update_timeout_mode(lk, path, flags,\n+\t\t\t\t\t\t timeout_ms, 0666);\n+}\n \n /*\n * Attempt to create a lockfile for the file at `path` and return a\n@@ -175,6 +192,13 @@ static inline int hold_lock_file_for_update(\n \treturn hold_lock_file_for_update_timeout(lk, path, flags, 0);\n }\n \n+static inline int hold_lock_file_for_update_mode(\n+\t\tstruct lock_file *lk, const char *path,\n+\t\tint flags, int mode)\n+{\n+\treturn hold_lock_file_for_update_timeout_mode(lk, path, flags, 0, mode);\n+}\n+\n /*\n * Return a nonzero value iff `lk` is currently locked.\n */\n-- \n2.26.0.113.ge9739cdccc\n\n"},{"id":"396346","messageId":"86cf29ce9c1e6dc1fc881458c18850c2893b092a.1588004647.git.me@ttaylorr.com","threadId":"53267","inReplyTo":"cover.1588004647.git.me@ttaylorr.com","subject":"[PATCH v2 3/4] commit-graph.c: write non-split graphs as read-only","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-04-27T16:28:02Z","receivedAt":"2020-04-27T16:28:08Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"In the previous commit, Git learned 'hold_lock_file_for_update_mode' to\nallow the caller to specify the permission bits (prior to further\nadjustment by the umask and shared repository permissions) used when\nacquiring a temporary file.\n\nUse this in the commit-graph machinery for writing a non-split graph to\nacquire an opened temporary file with permissions read-only permissions\nto match the split behavior. (In the split case, Git uses\ngit_mkstemp_mode' for each of the commit-graph layers with permission\nbits '0444').\n\nOne can notice this discrepancy when moving a non-split graph to be part\nof a new chain. This causes a commit-graph chain where all layers have\nread-only permission bits, except for the base layer, which is writable\nfor the current user.\n\nResolve this discrepancy by using the new\n'hold_lock_file_for_update_mode' and passing the desired permission\nbits.\n\nDoing so causes some test fallout in t5318 and t6600. In t5318, this\noccurs in tests that corrupt a commit-graph file by writing into it. For\nthese, 'chmod u+w'-ing the file beforehand resolves the issue. The\nadditional spot in 'corrupt_graph_verify' is necessary because of the\nextra 'git commit-graph write' beforehand (which *does* rewrite the\ncommit-graph file). In t6600, this is caused by copying a read-only\ncommit-graph file into place and then trying to replace it. For these,\nmake these files writable.\n\nSigned-off-by: Taylor Blau \n---\n commit-graph.c | 3 ++-\n t/t5318-commit-graph.sh | 11 ++++++++++-\n t/t6600-test-reach.sh | 2 ++\n 3 files changed, 14 insertions(+), 2 deletions(-)\n\ndiff --git a/commit-graph.c b/commit-graph.c\nindex f013a84e29..5b5047a7dd 100644\n--- a/commit-graph.c\n+++ b/commit-graph.c\n@@ -1388,7 +1388,8 @@ static int write_commit_graph_file(struct write_commit_graph_context *ctx)\n \n \t\tf = hashfd(fd, ctx->graph_name);\n \t} else {\n-\t\thold_lock_file_for_update(&lk, ctx->graph_name, LOCK_DIE_ON_ERROR);\n+\t\thold_lock_file_for_update_mode(&lk, ctx->graph_name,\n+\t\t\t\t\t LOCK_DIE_ON_ERROR, 0444);\n \t\tfd = lk.tempfile->fd;\n \t\tf = hashfd(lk.tempfile->fd, lk.tempfile->filename.buf);\n \t}\ndiff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh\nindex 9bf920ae17..fb0aae61c3 100755\n--- a/t/t5318-commit-graph.sh\n+++ b/t/t5318-commit-graph.sh\n@@ -96,6 +96,13 @@ test_expect_success 'write graph' '\n \tgraph_read_expect \"3\"\n '\n \n+test_expect_success POSIXPERM 'write graph has correct permissions' '\n+\ttest_path_is_file $objdir/info/commit-graph &&\n+\techo \"-r--r--r--\" >expect &&\n+\ttest_modebits $objdir/info/commit-graph >actual &&\n+\ttest_cmp expect actual\n+'\n+\n graph_git_behavior 'graph exists' full commits/3 commits/1\n \n test_expect_success 'Add more commits' '\n@@ -421,7 +428,8 @@ GRAPH_BYTE_FOOTER=$(($GRAPH_OCTOPUS_DATA_OFFSET + 4 * $NUM_OCTOPUS_EDGES))\n corrupt_graph_setup() {\n \tcd \"$TRASH_DIRECTORY/full\" &&\n \ttest_when_finished mv commit-graph-backup $objdir/info/commit-graph &&\n-\tcp $objdir/info/commit-graph commit-graph-backup\n+\tcp $objdir/info/commit-graph commit-graph-backup &&\n+\tchmod u+w $objdir/info/commit-graph\n }\n \n corrupt_graph_verify() {\n@@ -435,6 +443,7 @@ corrupt_graph_verify() {\n \tfi &&\n \tgit status --short &&\n \tGIT_TEST_COMMIT_GRAPH_DIE_ON_LOAD=true git commit-graph write &&\n+\tchmod u+w $objdir/info/commit-graph &&\n \tgit commit-graph verify\n }\n \ndiff --git a/t/t6600-test-reach.sh b/t/t6600-test-reach.sh\nindex b24d850036..475564bee7 100755\n--- a/t/t6600-test-reach.sh\n+++ b/t/t6600-test-reach.sh\n@@ -51,8 +51,10 @@ test_expect_success 'setup' '\n \tdone &&\n \tgit commit-graph write --reachable &&\n \tmv .git/objects/info/commit-graph commit-graph-full &&\n+\tchmod u+w commit-graph-full &&\n \tgit show-ref -s commit-5-5 | git commit-graph write --stdin-commits &&\n \tmv .git/objects/info/commit-graph commit-graph-half &&\n+\tchmod u+w commit-graph-half &&\n \tgit config core.commitGraph true\n '\n \n-- \n2.26.0.113.ge9739cdccc\n\n"},{"id":"396348","messageId":"f83437f130812d172167d335ba2d13b1545a9f58.1588004647.git.me@ttaylorr.com","threadId":"53267","inReplyTo":"cover.1588004647.git.me@ttaylorr.com","subject":"[PATCH v2 4/4] commit-graph.c: ensure graph layers respect core.sharedRepository","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-04-27T16:28:05Z","receivedAt":"2020-04-27T16:28:13Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"Non-layered commit-graphs use 'adjust_shared_perm' to make the\ncommit-graph file readable (or not) to a combination of the user, group,\nand others.\n\nCall 'adjust_shared_perm' for split-graph layers to make sure that these\nalso respect 'core.sharedRepository'. The 'commit-graph-chain' file\nalready respects this configuration since it uses\n'hold_lock_file_for_update' (which calls 'adjust_shared_perm' eventually\nin 'create_tempfile_mode').\n\nSuggested-by: Junio C Hamano \nSigned-off-by: Taylor Blau \n---\n commit-graph.c | 6 ++++++\n t/t5324-split-commit-graph.sh | 18 ++++++++++++++++++\n 2 files changed, 24 insertions(+)\n\ndiff --git a/commit-graph.c b/commit-graph.c\nindex 5b5047a7dd..d05a55901d 100644\n--- a/commit-graph.c\n+++ b/commit-graph.c\n@@ -1386,6 +1386,12 @@ static int write_commit_graph_file(struct write_commit_graph_context *ctx)\n \t\t\treturn -1;\n \t\t}\n \n+\t\tif (adjust_shared_perm(ctx->graph_name)) {\n+\t\t\terror(_(\"unable to adjust shared permissions for '%s'\"),\n+\t\t\t ctx->graph_name);\n+\t\t\treturn -1;\n+\t\t}\n+\n \t\tf = hashfd(fd, ctx->graph_name);\n \t} else {\n \t\thold_lock_file_for_update_mode(&lk, ctx->graph_name,\ndiff --git a/t/t5324-split-commit-graph.sh b/t/t5324-split-commit-graph.sh\nindex 53b2e6b455..61136c737f 100755\n--- a/t/t5324-split-commit-graph.sh\n+++ b/t/t5324-split-commit-graph.sh\n@@ -351,4 +351,22 @@ test_expect_success 'split across alternate where alternate is not split' '\n \ttest_cmp commit-graph .git/objects/info/commit-graph\n '\n \n+while read mode modebits\n+do\n+\ttest_expect_success POSIXPERM \"split commit-graph respects core.sharedrepository $mode\" '\n+\t\trm -rf $graphdir $infodir/commit-graph &&\n+\t\tgit reset --hard commits/1 &&\n+\t\ttest_config core.sharedrepository \"$mode\" &&\n+\t\tgit commit-graph write --split --reachable &&\n+\t\tls $graphdir/graph-*.graph >graph-files &&\n+\t\ttest_line_count = 1 graph-files &&\n+\t\techo \"$modebits\" >expect &&\n+\t\ttest_modebits $graphdir/graph-*.graph >actual &&\n+\t\ttest_cmp expect actual\n+\t'\n+done <<\\EOF\n+0666 -r--r--r--\n+0600 -r--------\n+EOF\n+\n test_done\n-- \n2.26.0.113.ge9739cdccc\n"},{"id":"396355","messageId":"20200427172111.GA58509@syl.local","threadId":"53267","inReplyTo":"f83437f130812d172167d335ba2d13b1545a9f58.1588004647.git.me@ttaylorr.com","subject":"Re: [PATCH v2 4/4] commit-graph.c: ensure graph layers respect core.sharedRepository","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-04-27T17:21:11Z","receivedAt":"2020-04-27T17:21:17Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Mon, Apr 27, 2020 at 10:28:05AM -0600, Taylor Blau wrote:\n> Non-layered commit-graphs use 'adjust_shared_perm' to make the\n> commit-graph file readable (or not) to a combination of the user, group,\n> and others.\n>\n> Call 'adjust_shared_perm' for split-graph layers to make sure that these\n> also respect 'core.sharedRepository'. The 'commit-graph-chain' file\n> already respects this configuration since it uses\n> 'hold_lock_file_for_update' (which calls 'adjust_shared_perm' eventually\n> in 'create_tempfile_mode').\n\nIt occurs to me that we might want to apply the same treatment to\n'commit-graph-chain's, too.\n\nJunio: I'm not sure if you want to apply the below in this series on\ntop, or if you'd prefer me send it as a separate series. Either way,\nhere's a patch to do just that:\n\n-- >8 --\n\nSubject: [PATCH] commit-graph.c: make 'commit-graph-chain's read-only\n\nIn a previous commit, we made incremental graph layers read-only by\nusing 'git_mkstemp_mode' with permissions '0444'.\n\nThere is no reason that 'commit-graph-chain's should be modifiable by\nthe user, since they are generated at a temporary location and then\natomically renamed into place.\n\nTo ensure that these files are read-only, too, use\n'hold_lock_file_for_update_mode' with the same read-only permission\nbits, and let the umask and 'adjust_shared_perm' take care of the rest.\n\nSigned-off-by: Taylor Blau \n---\n commit-graph.c | 3 ++-\n t/t5324-split-commit-graph.sh | 2 ++\n 2 files changed, 4 insertions(+), 1 deletion(-)\n\ndiff --git a/commit-graph.c b/commit-graph.c\nindex d05a55901d..b2dfd7701f 100644\n--- a/commit-graph.c\n+++ b/commit-graph.c\n@@ -1378,7 +1378,8 @@ static int write_commit_graph_file(struct write_commit_graph_context *ctx)\n \tif (ctx->split) {\n \t\tchar *lock_name = get_chain_filename(ctx->odb);\n\n-\t\thold_lock_file_for_update(&lk, lock_name, LOCK_DIE_ON_ERROR);\n+\t\thold_lock_file_for_update_mode(&lk, lock_name,\n+\t\t\t\t\t LOCK_DIE_ON_ERROR, 0444);\n\n \t\tfd = git_mkstemp_mode(ctx->graph_name, 0444);\n \t\tif (fd < 0) {\ndiff --git a/t/t5324-split-commit-graph.sh b/t/t5324-split-commit-graph.sh\nindex 61136c737f..a8b12c8110 100755\n--- a/t/t5324-split-commit-graph.sh\n+++ b/t/t5324-split-commit-graph.sh\n@@ -362,6 +362,8 @@ do\n \t\ttest_line_count = 1 graph-files &&\n \t\techo \"$modebits\" >expect &&\n \t\ttest_modebits $graphdir/graph-*.graph >actual &&\n+\t\ttest_cmp expect actual &&\n+\t\ttest_modebits $graphdir/commit-graph-chain >actual &&\n \t\ttest_cmp expect actual\n \t'\n done <<\\EOF\n--\n2.26.0.113.ge9739cdccc\n\n"},{"id":"396390","messageId":"20200427205808.GA1732530@coredump.intra.peff.net","threadId":"53267","inReplyTo":"20200427172111.GA58509@syl.local","subject":"Re: [PATCH v2 4/4] commit-graph.c: ensure graph layers respect core.sharedRepository","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-04-27T20:58:08Z","receivedAt":"2020-04-27T20:58:11Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Apr 27, 2020 at 11:21:11AM -0600, Taylor Blau wrote:\n\n> On Mon, Apr 27, 2020 at 10:28:05AM -0600, Taylor Blau wrote:\n> > Non-layered commit-graphs use 'adjust_shared_perm' to make the\n> > commit-graph file readable (or not) to a combination of the user, group,\n> > and others.\n> >\n> > Call 'adjust_shared_perm' for split-graph layers to make sure that these\n> > also respect 'core.sharedRepository'. The 'commit-graph-chain' file\n> > already respects this configuration since it uses\n> > 'hold_lock_file_for_update' (which calls 'adjust_shared_perm' eventually\n> > in 'create_tempfile_mode').\n> \n> It occurs to me that we might want to apply the same treatment to\n> 'commit-graph-chain's, too.\n\nYeah, perhaps. I think we've been fairly inconsistent about modes here.\n\nReally, just about _everything_ in .git is meant to be immutable,\nbecause we generally use rename() to update files atomically. And\nthere's no reason anybody should ever be opening them for writing.\n\nI think we started with dropping the write-bit on loose and packed\nobject files because those are extra-precious (even if everything else\nwere corrupted, your history, etc, is all still there). And certainly\nyou can't update them without invalidating their checksum trailers\nanyway.\n\nSo I think there's really three levels that could make sense:\n\n 1. Many files in .git should lose their write bits, because Git will\n never update them except through rename. This makes things safer\n against accidental changes, but more annoying when you do want to\n edit by hand. Probably .git/config should likely be exempted, as\n we'd encourage people to hand-edit even if it's not atomic. But\n having to chmod before hand-editing a packed-refs file while\n debugging is not a huge burden.\n\n 2. Any file written via hashfd() should be marked immutable. It cannot\n be edited without rewriting the whole contents and updating the\n trailer anyway. That would imply that commit-graph and chain files\n should be immutable, but the commit-graph-chain file should not.\n\n 3. Everything except actual object files should retain their write\n bit. It's a minor annoyance when touching the repo by hand (e.g.,\n \"rm\" is sometimes pickier even about deletion), and it's not like\n there's a rash of people accidentally overwriting their refs/\n files (they're just as likely to screw themselves by deleting\n them).\n\nI admit I don't overly care much between the three. And your patches\nlook fine to me, as they're just making the commit-graph code consistent\nwith itself (and that inconsistency is wrong under any of the mental\nmodels above ;) ).\n\nThe exception is the final patch, which is an actual bug-fix for people\nusing core.sharedRepository. I suspect the feature is used infrequently\nenough that nobody noticed.\n\n-Peff\n"},{"id":"396411","messageId":"xmqqr1w85vtq.fsf@gitster.c.googlers.com","threadId":"53267","inReplyTo":"86cf29ce9c1e6dc1fc881458c18850c2893b092a.1588004647.git.me@ttaylorr.com","subject":"Re: [PATCH v2 3/4] commit-graph.c: write non-split graphs as read-only","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-04-27T23:54:09Z","receivedAt":"2020-04-27T23:54:15Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Taylor Blau writes:\n\n> In the previous commit, Git learned 'hold_lock_file_for_update_mode' to\n> allow the caller to specify the permission bits (prior to further\n> adjustment by the umask and shared repository permissions) used when\n> acquiring a temporary file.\n>\n> Use this in the commit-graph machinery for writing a non-split graph to\n> acquire an opened temporary file with permissions read-only permissions\n> to match the split behavior. (In the split case, Git uses\n> git_mkstemp_mode' for each of the commit-graph layers with permission\n> bits '0444').\n>\n> One can notice this discrepancy when moving a non-split graph to be part\n> of a new chain. This causes a commit-graph chain where all layers have\n> read-only permission bits, except for the base layer, which is writable\n> for the current user.\n>\n> Resolve this discrepancy by using the new\n> 'hold_lock_file_for_update_mode' and passing the desired permission\n> bits.\n>\n> Doing so causes some test fallout in t5318 and t6600. In t5318, this\n> occurs in tests that corrupt a commit-graph file by writing into it. For\n> these, 'chmod u+w'-ing the file beforehand resolves the issue. The\n> additional spot in 'corrupt_graph_verify' is necessary because of the\n> extra 'git commit-graph write' beforehand (which *does* rewrite the\n> commit-graph file). In t6600, this is caused by copying a read-only\n> commit-graph file into place and then trying to replace it. For these,\n> make these files writable.\n>\n> Signed-off-by: Taylor Blau \n> ---\n> commit-graph.c | 3 ++-\n> t/t5318-commit-graph.sh | 11 ++++++++++-\n> t/t6600-test-reach.sh | 2 ++\n> 3 files changed, 14 insertions(+), 2 deletions(-)\n\nThis step, queued as 3a5c7d70 (commit-graph.c: write non-split\ngraphs as read-only, 2020-04-27), starts failing 5318#9 at least for\nme. Do we need to loosen umask while running this test to something\nnot more strict than 022 or something silly like that?\n\nHere is what I'll use as a workaround for today's pushout.\n\ncommit f062d1c028bcc839f961e8904c38c476b9deeec3\nAuthor: Junio C Hamano \nDate: Mon Apr 27 16:50:30 2020 -0700\n\n SQUASH??? force known umask if you are going to check the resulting mode bits\n\ndiff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh\nindex fb0aae61c3..901eb3ecfb 100755\n--- a/t/t5318-commit-graph.sh\n+++ b/t/t5318-commit-graph.sh\n@@ -12,6 +12,10 @@ test_expect_success 'setup full repo' '\n \ttest_oid_init\n '\n \n+test_expect_success POSIXPERM 'tweak umask for modebit tests' '\n+\tumask 022\n+'\n+\n test_expect_success 'verify graph with no graph file' '\n \tcd \"$TRASH_DIRECTORY/full\" &&\n \tgit commit-graph verify\n"},{"id":"396412","messageId":"20200427235935.GA14984@syl.local","threadId":"53267","inReplyTo":"xmqqr1w85vtq.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v2 3/4] commit-graph.c: write non-split graphs as read-only","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-04-27T23:59:35Z","receivedAt":"2020-04-27T23:59:41Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Mon, Apr 27, 2020 at 04:54:09PM -0700, Junio C Hamano wrote:\n> This step, queued as 3a5c7d70 (commit-graph.c: write non-split\n> graphs as read-only, 2020-04-27), starts failing 5318#9 at least for\n> me. Do we need to loosen umask while running this test to something\n> not more strict than 022 or something silly like that?\n>\n> Here is what I'll use as a workaround for today's pushout.\n>\n> commit f062d1c028bcc839f961e8904c38c476b9deeec3\n> Author: Junio C Hamano \n> Date: Mon Apr 27 16:50:30 2020 -0700\n>\n> SQUASH??? force known umask if you are going to check the resulting mode bits\n>\n> diff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh\n> index fb0aae61c3..901eb3ecfb 100755\n> --- a/t/t5318-commit-graph.sh\n> +++ b/t/t5318-commit-graph.sh\n> @@ -12,6 +12,10 @@ test_expect_success 'setup full repo' '\n> \ttest_oid_init\n> '\n>\n> +test_expect_success POSIXPERM 'tweak umask for modebit tests' '\n> +\tumask 022\n> +'\n> +\n> test_expect_success 'verify graph with no graph file' '\n> \tcd \"$TRASH_DIRECTORY/full\" &&\n> \tgit commit-graph verify\n\nLooks good to me; this is definitely necessary. For what it's worth, it\npasses for me, but my system may not have as restrictive a umask as\nothers.\n\nI'd be happy to re-send this patch, but alternatively if you want to\njust squash this in, I'd be just as happy.\n\nThanks,\nTaylor\n"},{"id":"396418","messageId":"xmqqd07s5ucx.fsf@gitster.c.googlers.com","threadId":"53267","inReplyTo":"20200427235935.GA14984@syl.local","subject":"Re: [PATCH v2 3/4] commit-graph.c: write non-split graphs as read-only","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-04-28T00:25:50Z","receivedAt":"2020-04-28T00:25:54Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Taylor Blau writes:\n\n> Looks good to me; this is definitely necessary. For what it's worth, it\n> passes for me, but my system may not have as restrictive a umask as\n> others.\n\nNote that umask is not a system thing, but personal. When I was\nwith smaller company, I used to have 002 as my umask, but these days\nI use 077.\n\n> I'd be happy to re-send this patch, but alternatively if you want to\n> just squash this in, I'd be just as happy.\n\nI'll keep it queued, until we need to replace it with an undated\nseries.\n\nThanks.\n"},{"id":"396428","messageId":"20200428033438.GA2369457@coredump.intra.peff.net","threadId":"53267","inReplyTo":"20200427235935.GA14984@syl.local","subject":"Re: [PATCH v2 3/4] commit-graph.c: write non-split graphs as read-only","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-04-28T03:34:38Z","receivedAt":"2020-04-28T03:34:41Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Apr 27, 2020 at 05:59:35PM -0600, Taylor Blau wrote:\n\n> > diff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh\n> > index fb0aae61c3..901eb3ecfb 100755\n> > --- a/t/t5318-commit-graph.sh\n> > +++ b/t/t5318-commit-graph.sh\n> > @@ -12,6 +12,10 @@ test_expect_success 'setup full repo' '\n> > \ttest_oid_init\n> > '\n> >\n> > +test_expect_success POSIXPERM 'tweak umask for modebit tests' '\n> > +\tumask 022\n> > +'\n> > +\n> > test_expect_success 'verify graph with no graph file' '\n> > \tcd \"$TRASH_DIRECTORY/full\" &&\n> > \tgit commit-graph verify\n> \n> Looks good to me; this is definitely necessary. For what it's worth, it\n> passes for me, but my system may not have as restrictive a umask as\n> others.\n\nIf we're just doing this for a single test, perhaps it would be better\nto set the umask in that test (perhaps even in a subshell to avoid\ntouching other tests). I guess that's a little awkward here because the\nwrite and the mode-check happen in separate snippets.\n\nOr going in the opposite direction: if we think that covering the rest\nof the test suite with a diversity of umasks isn't worthwhile, we could\njust set \"umask\" in test-lib.sh. That would solve this problem and any\nfuture ones.\n\nI also wondered if it would be simpler to just limit the scope of the\ntest, like so:\n\ndiff --git b/t/t5318-commit-graph.sh a/t/t5318-commit-graph.sh\nindex fb0aae61c3..5f1c746ad1 100755\n--- b/t/t5318-commit-graph.sh\n+++ a/t/t5318-commit-graph.sh\n@@ -98,9 +98,10 @@ test_expect_success 'write graph' '\n \n test_expect_success POSIXPERM 'write graph has correct permissions' '\n \ttest_path_is_file $objdir/info/commit-graph &&\n-\techo \"-r--r--r--\" >expect &&\n \ttest_modebits $objdir/info/commit-graph >actual &&\n-\ttest_cmp expect actual\n+\t# check only user mode bits, as we do not want to rely on\n+\t# test environment umask\n+\tgrep ^-r-- actual\n '\n \n graph_git_behavior 'graph exists' full commits/3 commits/1\n\nbut maybe there's some value in checking the whole thing.\n\n-Peff\n"},{"id":"396474","messageId":"xmqqk11z4ksl.fsf@gitster.c.googlers.com","threadId":"53267","inReplyTo":"20200428033438.GA2369457@coredump.intra.peff.net","subject":"Re: [PATCH v2 3/4] commit-graph.c: write non-split graphs as read-only","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-04-28T16:50:02Z","receivedAt":"2020-04-28T16:50:09Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> If we're just doing this for a single test, perhaps it would be better\n> to set the umask in that test (perhaps even in a subshell to avoid\n> touching other tests). I guess that's a little awkward here because the\n> write and the mode-check happen in separate snippets.\n\nYes, and we cannot afford to place the writing side under POSIXPERM\nprerequisite.\n\n> Or going in the opposite direction: if we think that covering the rest\n> of the test suite with a diversity of umasks isn't worthwhile, we could\n> just set \"umask\" in test-lib.sh. That would solve this problem and any\n> future ones.\n\nSeen from the point of view of giving us a stable testing\nenvironment, it certainly feels like an easy and simple thing to do.\nI do not offhand see any downsides in that approach.\n\nOn the other hand, we use and rely on test-specified umask only in a\nfew tests (t0001, t1301 and t1304). Perhaps we should discourage\ntests outside these to rely too heavily on exact perm bits?\n\nFor example, I wonder if we should have used\n\n\ttest -r commit-graph && ! test -w commit-graph \n\nto ensure the file is read-only to the user who is testing, instead\nof relying on parsing \"ls -l\" output, which IIRC has bitten us with\nxattr and forced us to revise test_modebits helper in 5610e3b0 (Fix\ntestcase failure when extended attributes are in use, 2008-10-19).\nThat would make the test require SANITY instead, though.\n\n> I also wondered if it would be simpler to just limit the scope of the\n> test, like so:\n> ...\n> +\t# check only user mode bits, as we do not want to rely on\n> +\t# test environment umask\n> +\tgrep ^-r-- actual\n> '\n> ...\n> but maybe there's some value in checking the whole thing.\n\nYeah, I guess we are wondering about the same thing.\n\nAmong various approaches on plate, my preference is to use \"umask\n022\" around the place where we prepare the $TRASH_DIRECTORY and do\nso only when POSIXPERM is there in the test-lib.sh. I do not know\nif we should do so before or after creating the $TRASH_DIRECTORY;\nmy gut feeling is that in the ideal world, we should be able to\n\n - create trash directory\n\n - use the directory to automatically figure out POSIXPERM\n\n - if POSIXPERM is set, use umask 022 and chmod og=rx the trash\n directory\n\nAutomatically figuring out POSIXPERM the above approach shoots for\nis a much larger change, so I am not in a haste to implement it and\nit may be OK to only do \"umask 022\" after we set POSIXPERM for\neverybody but MINGW at least as the initial cut, but that would mean\nwe would run for quite a long time with the testing user's umask\nduring the setup process, which is unsatisfying.\n"},{"id":"396501","messageId":"20200428205913.GC4000@coredump.intra.peff.net","threadId":"53267","inReplyTo":"xmqqk11z4ksl.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v2 3/4] commit-graph.c: write non-split graphs as read-only","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-04-28T20:59:13Z","receivedAt":"2020-04-28T20:59:15Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Apr 28, 2020 at 09:50:02AM -0700, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > If we're just doing this for a single test, perhaps it would be better\n> > to set the umask in that test (perhaps even in a subshell to avoid\n> > touching other tests). I guess that's a little awkward here because the\n> > write and the mode-check happen in separate snippets.\n> \n> Yes, and we cannot afford to place the writing side under POSIXPERM\n> prerequisite.\n\nDo we need POSIXPERM just to call umask?\n\nWe call it unconditionally in t1304, for example. I could certainly\nbelieve it doesn't do anything useful or predictable on other systems,\nbut it would not surprise me if it is a silent noop. It might return\nnon-zero, though (the call in t1304 is not inside a test snippet).\n\n> Among various approaches on plate, my preference is to use \"umask\n> 022\" around the place where we prepare the $TRASH_DIRECTORY and do\n> so only when POSIXPERM is there in the test-lib.sh. I do not know\n> if we should do so before or after creating the $TRASH_DIRECTORY;\n> my gut feeling is that in the ideal world, we should be able to\n> \n> - create trash directory\n> \n> - use the directory to automatically figure out POSIXPERM\n> \n> - if POSIXPERM is set, use umask 022 and chmod og=rx the trash\n> directory\n\nI don't think we do any actual filesystem tests for POSIXPERM. It's\npurely based on \"uname -s\", and we could check it much earlier. So\nunless actually probing the filesystem is worth doing, we could just\npunt on that part easily.\n\nThat said, I think this does get complicated when interacting with\nt1304, for example, which explicitly creates an 077 umask for the trash\ndirectory.\n\nThis is looking like a much deeper rabbit hole than it's worth going\ndown. I think the pragmatic thing is to just stick a \"umask 022\" near\nthe new test (or possibly \"test_might_fail umask 022\" inside the\ncommit-graph writing test).\n\n-Peff\n"},{"id":"396505","messageId":"xmqqd07r1fu6.fsf@gitster.c.googlers.com","threadId":"53267","inReplyTo":"20200428205913.GC4000@coredump.intra.peff.net","subject":"Re: [PATCH v2 3/4] commit-graph.c: write non-split graphs as read-only","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-04-28T21:05:21Z","receivedAt":"2020-04-28T21:05:31Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Tue, Apr 28, 2020 at 09:50:02AM -0700, Junio C Hamano wrote:\n>\n>> Jeff King writes:\n>> \n>> > If we're just doing this for a single test, perhaps it would be better\n>> > to set the umask in that test (perhaps even in a subshell to avoid\n>> > touching other tests). I guess that's a little awkward here because the\n>> > write and the mode-check happen in separate snippets.\n>> \n>> Yes, and we cannot afford to place the writing side under POSIXPERM\n>> prerequisite.\n>\n> Do we need POSIXPERM just to call umask?\n\nI checked \"git grep umask t/\" hits, and I thought everybody was\nusing it inside POSIXPERM.\n\n> We call it unconditionally in t1304, for example. I could certainly\n> believe it doesn't do anything useful or predictable on other systems,\n> but it would not surprise me if it is a silent noop. It might return\n> non-zero, though (the call in t1304 is not inside a test snippet).\n\nThat is sloppy, but it might be deliberate that it does not check\nthe result?\n\n> I don't think we do any actual filesystem tests for POSIXPERM. It's\n> purely based on \"uname -s\", and we could check it much earlier. So\n> unless actually probing the filesystem is worth doing, we could just\n> punt on that part easily.\n\nYup.\n\n> That said, I think this does get complicated when interacting with\n> t1304, for example, which explicitly creates an 077 umask for the trash\n> directory.\n>\n> This is looking like a much deeper rabbit hole than it's worth going\n> down. I think the pragmatic thing is to just stick a \"umask 022\" near\n> the new test (or possibly \"test_might_fail umask 022\" inside the\n> commit-graph writing test).\n\nI think the most pragmatic would be to just squash in what is\nalready there ;-)\n\n"},{"id":"396507","messageId":"20200428210821.GF4000@coredump.intra.peff.net","threadId":"53267","inReplyTo":"xmqqd07r1fu6.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v2 3/4] commit-graph.c: write non-split graphs as read-only","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-04-28T21:08:21Z","receivedAt":"2020-04-28T21:08:23Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Apr 28, 2020 at 02:05:21PM -0700, Junio C Hamano wrote:\n\n> > This is looking like a much deeper rabbit hole than it's worth going\n> > down. I think the pragmatic thing is to just stick a \"umask 022\" near\n> > the new test (or possibly \"test_might_fail umask 022\" inside the\n> > commit-graph writing test).\n> \n> I think the most pragmatic would be to just squash in what is\n> already there ;-)\n\nThat is OK with me. :)\n\n-Peff\n"},{"id":"396512","messageId":"20200428214413.GA47690@syl.local","threadId":"53267","inReplyTo":"20200428210821.GF4000@coredump.intra.peff.net","subject":"Re: [PATCH v2 3/4] commit-graph.c: write non-split graphs as read-only","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-04-28T21:44:13Z","receivedAt":"2020-04-28T21:44:20Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Tue, Apr 28, 2020 at 05:08:21PM -0400, Jeff King wrote:\n> On Tue, Apr 28, 2020 at 02:05:21PM -0700, Junio C Hamano wrote:\n>\n> > > This is looking like a much deeper rabbit hole than it's worth going\n> > > down. I think the pragmatic thing is to just stick a \"umask 022\" near\n> > > the new test (or possibly \"test_might_fail umask 022\" inside the\n> > > commit-graph writing test).\n> >\n> > I think the most pragmatic would be to just squash in what is\n> > already there ;-)\n>\n> That is OK with me. :)\n\nThanks for an interesting discussion. I squashed Junio's fix into the\nthird patch, but the fourth patch suffers from the same problem (so I\nstuck another POSIXPERM test to tweak the umask there, too).\n\nWhat do you want to do about the final patch that I stuck on the end of\nthis series in [1]? If I don't hear from anybody, I'll send it as 5/5 in\nv3 and we can feel free to not apply it if it's controversial.\n\n> -Peff\n\nThanks,\nTaylor\n\n[1]: https://lore.kernel.org/git/20200427172111.GA58509@syl.local/\n"},{"id":"396514","messageId":"20200428215815.GA8526@coredump.intra.peff.net","threadId":"53267","inReplyTo":"20200428214413.GA47690@syl.local","subject":"Re: [PATCH v2 3/4] commit-graph.c: write non-split graphs as read-only","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-04-28T21:58:15Z","receivedAt":"2020-04-28T21:58:18Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Apr 28, 2020 at 03:44:13PM -0600, Taylor Blau wrote:\n\n> What do you want to do about the final patch that I stuck on the end of\n> this series in [1]? If I don't hear from anybody, I'll send it as 5/5 in\n> v3 and we can feel free to not apply it if it's controversial.\n\nI have to admit I don't care that much either way about it (see my\nearlier response on three mental models). I'm happy for you or Junio to\ndecide. :)\n\n-Peff\n"},{"id":"396517","messageId":"xmqqmu6vyz4q.fsf@gitster.c.googlers.com","threadId":"53267","inReplyTo":"20200428215815.GA8526@coredump.intra.peff.net","subject":"Re: [PATCH v2 3/4] commit-graph.c: write non-split graphs as read-only","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-04-28T23:22:13Z","receivedAt":"2020-04-28T23:22:18Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Tue, Apr 28, 2020 at 03:44:13PM -0600, Taylor Blau wrote:\n>\n>> What do you want to do about the final patch that I stuck on the end of\n>> this series in [1]? If I don't hear from anybody, I'll send it as 5/5 in\n>> v3 and we can feel free to not apply it if it's controversial.\n>\n> I have to admit I don't care that much either way about it (see my\n> earlier response on three mental models). I'm happy for you or Junio to\n> decide. :)\n\nMy gut feeling is that our longer term goal (if we had timeperiod\nduring which the codebase is quiescent enough and infinite energy to\ndedicate on code clean-up) among one or your options should be to\nconsistently create files that are rewritten-and-renamed read-only,\nto discourage casual tampering, so I am OK with that 5th patch.\n\nHaving said that, I suspect that Derrick and friends are larger\nstakeholders in the \"chain\" file, so I'd prefer to see us basing\nthe choice on their input.\n\nThanks.\n\n"},{"id":"396544","messageId":"c88a9169-47fd-2fdf-823e-d69d59f406d1@gmail.com","threadId":"53267","inReplyTo":"xmqqmu6vyz4q.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v2 3/4] commit-graph.c: write non-split graphs as read-only","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2020-04-29T11:52:00Z","receivedAt":"2020-04-29T11:52:05Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 4/28/2020 7:22 PM, Junio C Hamano wrote:\n> Jeff King writes:\n> \n>> On Tue, Apr 28, 2020 at 03:44:13PM -0600, Taylor Blau wrote:\n>>\n>>> What do you want to do about the final patch that I stuck on the end of\n>>> this series in [1]? If I don't hear from anybody, I'll send it as 5/5 in\n>>> v3 and we can feel free to not apply it if it's controversial.\n>>\n>> I have to admit I don't care that much either way about it (see my\n>> earlier response on three mental models). I'm happy for you or Junio to\n>> decide. :)\n> \n> My gut feeling is that our longer term goal (if we had timeperiod\n> during which the codebase is quiescent enough and infinite energy to\n> dedicate on code clean-up) among one or your options should be to\n> consistently create files that are rewritten-and-renamed read-only,\n> to discourage casual tampering, so I am OK with that 5th patch.\n> \n> Having said that, I suspect that Derrick and friends are larger\n> stakeholders in the \"chain\" file, so I'd prefer to see us basing\n> the choice on their input.\n\nI'm happy with how this discussion has gone. I'm sure the only reason\nfor the permissions I wrote was because I found them somewhere else in\nthe codebase and I copied them from there. Memory is fuzzy, but I can\nguarantee the deviation from the norm was not in purpose.\n\nThanks,\n-Stolee\n"},{"id":"396579","messageId":"cover.1588181626.git.me@ttaylorr.com","threadId":"53267","inReplyTo":"cover.1587422630.git.me@ttaylorr.com","subject":"[PATCH v3 0/5] commit-graph: write non-split graphs as read-only","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-04-29T17:36:26Z","receivedAt":"2020-04-29T17:36:31Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"Hi,\n\nHere's a brief reroll to incorporate suggestions from Junio and Peff in\nthe sub-thread beginning at [1]. Not much at all has changed since v2,\nwith the exception of:\n\n * fixing the aforementioned issue by sprinkling in some 'umask 022's\n in throughout the new tests\n\n * adding in a new final patch to apply the same treatment to\n the 'commit-graph-chain' file\n\n[1]: https://lore.kernel.org/git/xmqqr1w85vtq.fsf@gitster.c.googlers.com/\n\nTaylor Blau (5):\n tempfile.c: introduce 'create_tempfile_mode'\n lockfile.c: introduce 'hold_lock_file_for_update_mode'\n commit-graph.c: write non-split graphs as read-only\n commit-graph.c: ensure graph layers respect core.sharedRepository\n commit-graph.c: make 'commit-graph-chain's read-only\n\n commit-graph.c | 12 ++++++++++--\n lockfile.c | 18 ++++++++++--------\n lockfile.h | 32 ++++++++++++++++++++++++++++----\n t/t5318-commit-graph.sh | 15 ++++++++++++++-\n t/t5324-split-commit-graph.sh | 24 ++++++++++++++++++++++++\n t/t6600-test-reach.sh | 2 ++\n tempfile.c | 6 +++---\n tempfile.h | 10 +++++++++-\n 8 files changed, 100 insertions(+), 19 deletions(-)\n\nRange-diff against v2:\n1: 03c975b0bd = 1: 03c975b0bd tempfile.c: introduce 'create_tempfile_mode'\n2: c1c84552bc = 2: c1c84552bc lockfile.c: introduce 'hold_lock_file_for_update_mode'\n3: 86cf29ce9c ! 3: 8d5503d2e6 commit-graph.c: write non-split graphs as read-only\n @@ Commit message\n commit-graph file into place and then trying to replace it. For these,\n make these files writable.\n\n + Helped-by: Junio C Hamano \n Signed-off-by: Taylor Blau \n\n ## commit-graph.c ##\n @@ commit-graph.c: static int write_commit_graph_file(struct write_commit_graph_con\n \t}\n\n ## t/t5318-commit-graph.sh ##\n +@@ t/t5318-commit-graph.sh: test_expect_success 'setup full repo' '\n + \ttest_oid_init\n + '\n +\n ++test_expect_success POSIXPERM 'tweak umask for modebit tests' '\n ++\tumask 022\n ++'\n ++\n + test_expect_success 'verify graph with no graph file' '\n + \tcd \"$TRASH_DIRECTORY/full\" &&\n + \tgit commit-graph verify\n @@ t/t5318-commit-graph.sh: test_expect_success 'write graph' '\n \tgraph_read_expect \"3\"\n '\n4: f83437f130 ! 4: 4b74e23af2 commit-graph.c: ensure graph layers respect core.sharedRepository\n @@ commit-graph.c: static int write_commit_graph_file(struct write_commit_graph_con\n \t\thold_lock_file_for_update_mode(&lk, ctx->graph_name,\n\n ## t/t5324-split-commit-graph.sh ##\n +@@ t/t5324-split-commit-graph.sh: graph_read_expect() {\n + \ttest_cmp expect output\n + }\n +\n ++test_expect_success POSIXPERM 'tweak umask for modebit tests' '\n ++\tumask 022\n ++'\n ++\n + test_expect_success 'create commits and write commit-graph' '\n + \tfor i in $(test_seq 3)\n + \tdo\n @@ t/t5324-split-commit-graph.sh: test_expect_success 'split across alternate where alternate is not split' '\n \ttest_cmp commit-graph .git/objects/info/commit-graph\n '\n-: ---------- > 5: 864c916067 commit-graph.c: make 'commit-graph-chain's read-only\n--\n2.26.0.113.ge9739cdccc\n"},{"id":"396580","messageId":"03c975b0bd0cd3dd5d693187f12f45fd7565016c.1588181626.git.me@ttaylorr.com","threadId":"53267","inReplyTo":"cover.1588181626.git.me@ttaylorr.com","subject":"[PATCH v3 1/5] tempfile.c: introduce 'create_tempfile_mode'","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-04-29T17:36:31Z","receivedAt":"2020-04-29T17:36:35Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"In the next patch, 'hold_lock_file_for_update' will gain an additional\n'mode' parameter to specify permissions for the associated temporary\nfile.\n\nSince the lockfile.c machinery uses 'create_tempfile' which always\ncreates a temporary file with global read-write permissions, introduce a\nvariant here that allows specifying the mode.\n\nNote that the mode given to 'create_tempfile_mode' is not guaranteed to\nbe written to disk, since it is subject to both the umask and\n'core.sharedRepository'.\n\nArguably, all temporary files should have permission 0444, since they\nare likely to be renamed into place and then not written to again. This\nis a much larger change than we may want to take on in this otherwise\nsmall patch, so for the time being, make 'create_tempfile' behave as it\nhas always done by inlining it to 'create_tempfile_mode' with mode set\nto '0666'.\n\nSigned-off-by: Taylor Blau \n---\n tempfile.c | 6 +++---\n tempfile.h | 10 +++++++++-\n 2 files changed, 12 insertions(+), 4 deletions(-)\n\ndiff --git a/tempfile.c b/tempfile.c\nindex d43ad8c191..94aa18f3f7 100644\n--- a/tempfile.c\n+++ b/tempfile.c\n@@ -130,17 +130,17 @@ static void deactivate_tempfile(struct tempfile *tempfile)\n }\n \n /* Make sure errno contains a meaningful value on error */\n-struct tempfile *create_tempfile(const char *path)\n+struct tempfile *create_tempfile_mode(const char *path, int mode)\n {\n \tstruct tempfile *tempfile = new_tempfile();\n \n \tstrbuf_add_absolute_path(&tempfile->filename, path);\n \ttempfile->fd = open(tempfile->filename.buf,\n-\t\t\t O_RDWR | O_CREAT | O_EXCL | O_CLOEXEC, 0666);\n+\t\t\t O_RDWR | O_CREAT | O_EXCL | O_CLOEXEC, mode);\n \tif (O_CLOEXEC && tempfile->fd < 0 && errno == EINVAL)\n \t\t/* Try again w/o O_CLOEXEC: the kernel might not support it */\n \t\ttempfile->fd = open(tempfile->filename.buf,\n-\t\t\t\t O_RDWR | O_CREAT | O_EXCL, 0666);\n+\t\t\t\t O_RDWR | O_CREAT | O_EXCL, mode);\n \tif (tempfile->fd < 0) {\n \t\tdeactivate_tempfile(tempfile);\n \t\treturn NULL;\ndiff --git a/tempfile.h b/tempfile.h\nindex cddda0a33c..4de3bc77d2 100644\n--- a/tempfile.h\n+++ b/tempfile.h\n@@ -88,8 +88,16 @@ struct tempfile {\n * Attempt to create a temporary file at the specified `path`. Return\n * a tempfile (whose \"fd\" member can be used for writing to it), or\n * NULL on error. It is an error if a file already exists at that path.\n+ * Note that `mode` will be further modified by the umask, and possibly\n+ * `core.sharedRepository`, so it is not guaranteed to have the given\n+ * mode.\n */\n-struct tempfile *create_tempfile(const char *path);\n+struct tempfile *create_tempfile_mode(const char *path, int mode);\n+\n+static inline struct tempfile *create_tempfile(const char *path)\n+{\n+\treturn create_tempfile_mode(path, 0666);\n+}\n \n /*\n * Register an existing file as a tempfile, meaning that it will be\n-- \n2.26.0.113.ge9739cdccc\n\n"},{"id":"396581","messageId":"c1c84552bc69218299ce182388b6a888b3c71056.1588181626.git.me@ttaylorr.com","threadId":"53267","inReplyTo":"cover.1588181626.git.me@ttaylorr.com","subject":"[PATCH v3 2/5] lockfile.c: introduce 'hold_lock_file_for_update_mode'","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-04-29T17:36:35Z","receivedAt":"2020-04-29T17:36:39Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"We use 'hold_lock_file_for_update' (and the '_timeout') variant to\nacquire a lock when updating references, the commit-graph file, and so\non.\n\nIn particular, the commit-graph machinery uses this to acquire a\ntemporary file that is used to write a non-split commit-graph. In a\nsubsequent commit, an issue in the commit-graph machinery produces\ngraph files that have a different permission based on whether or not\nthey are part of a multi-layer graph will be addressed.\n\nTo do so, the commit-graph machinery will need a version of\n'hold_lock_file_for_update' that takes the permission bits from the\ncaller.\n\nIntroduce such a function in this patch for both the\n'hold_lock_file_for_update' and 'hold_lock_file_for_update_timeout'\nfunctions, and leave the existing functions alone by inlining their\ndefinitions in terms of the new mode variants.\n\nNote that, like in the previous commit, 'hold_lock_file_for_update_mode'\nis not guarenteed to set the given mode, since it may be modified by\nboth the umask and 'core.sharedRepository'.\n\nNote also that even though the commit-graph machinery only calls\n'hold_lock_file_for_update', that this is defined in terms of\n'hold_lock_file_for_update_timeout', and so both need an additional mode\nparameter here.\n\nSigned-off-by: Taylor Blau \n---\n lockfile.c | 18 ++++++++++--------\n lockfile.h | 32 ++++++++++++++++++++++++++++----\n 2 files changed, 38 insertions(+), 12 deletions(-)\n\ndiff --git a/lockfile.c b/lockfile.c\nindex 8e8ab4f29f..cc9a4b8428 100644\n--- a/lockfile.c\n+++ b/lockfile.c\n@@ -70,7 +70,8 @@ static void resolve_symlink(struct strbuf *path)\n }\n \n /* Make sure errno contains a meaningful value on error */\n-static int lock_file(struct lock_file *lk, const char *path, int flags)\n+static int lock_file(struct lock_file *lk, const char *path, int flags,\n+\t\t int mode)\n {\n \tstruct strbuf filename = STRBUF_INIT;\n \n@@ -79,7 +80,7 @@ static int lock_file(struct lock_file *lk, const char *path, int flags)\n \t\tresolve_symlink(&filename);\n \n \tstrbuf_addstr(&filename, LOCK_SUFFIX);\n-\tlk->tempfile = create_tempfile(filename.buf);\n+\tlk->tempfile = create_tempfile_mode(filename.buf, mode);\n \tstrbuf_release(&filename);\n \treturn lk->tempfile ? lk->tempfile->fd : -1;\n }\n@@ -99,7 +100,7 @@ static int lock_file(struct lock_file *lk, const char *path, int flags)\n * exactly once. If timeout_ms is -1, try indefinitely.\n */\n static int lock_file_timeout(struct lock_file *lk, const char *path,\n-\t\t\t int flags, long timeout_ms)\n+\t\t\t int flags, long timeout_ms, int mode)\n {\n \tint n = 1;\n \tint multiplier = 1;\n@@ -107,7 +108,7 @@ static int lock_file_timeout(struct lock_file *lk, const char *path,\n \tstatic int random_initialized = 0;\n \n \tif (timeout_ms == 0)\n-\t\treturn lock_file(lk, path, flags);\n+\t\treturn lock_file(lk, path, flags, mode);\n \n \tif (!random_initialized) {\n \t\tsrand((unsigned int)getpid());\n@@ -121,7 +122,7 @@ static int lock_file_timeout(struct lock_file *lk, const char *path,\n \t\tlong backoff_ms, wait_ms;\n \t\tint fd;\n \n-\t\tfd = lock_file(lk, path, flags);\n+\t\tfd = lock_file(lk, path, flags, mode);\n \n \t\tif (fd >= 0)\n \t\t\treturn fd; /* success */\n@@ -169,10 +170,11 @@ NORETURN void unable_to_lock_die(const char *path, int err)\n }\n \n /* This should return a meaningful errno on failure */\n-int hold_lock_file_for_update_timeout(struct lock_file *lk, const char *path,\n-\t\t\t\t int flags, long timeout_ms)\n+int hold_lock_file_for_update_timeout_mode(struct lock_file *lk,\n+\t\t\t\t\t const char *path, int flags,\n+\t\t\t\t\t long timeout_ms, int mode)\n {\n-\tint fd = lock_file_timeout(lk, path, flags, timeout_ms);\n+\tint fd = lock_file_timeout(lk, path, flags, timeout_ms, mode);\n \tif (fd < 0) {\n \t\tif (flags & LOCK_DIE_ON_ERROR)\n \t\t\tunable_to_lock_die(path, errno);\ndiff --git a/lockfile.h b/lockfile.h\nindex 9843053ce8..db93e6ba73 100644\n--- a/lockfile.h\n+++ b/lockfile.h\n@@ -90,6 +90,15 @@\n * functions. In particular, the state diagram and the cleanup\n * machinery are all implemented in the tempfile module.\n *\n+ * Permission bits\n+ * ---------------\n+ *\n+ * If you call either `hold_lock_file_for_update_mode` or\n+ * `hold_lock_file_for_update_timeout_mode`, you can specify a suggested\n+ * mode for the underlying temporary file. Note that the file isn't\n+ * guaranteed to have this exact mode, since it may be limited by either\n+ * the umask, 'core.sharedRepository', or both. See `adjust_shared_perm`\n+ * for more.\n *\n * Error handling\n * --------------\n@@ -156,12 +165,20 @@ struct lock_file {\n * file descriptor for writing to it, or -1 on error. If the file is\n * currently locked, retry with quadratic backoff for at least\n * timeout_ms milliseconds. If timeout_ms is 0, try exactly once; if\n- * timeout_ms is -1, retry indefinitely. The flags argument and error\n- * handling are described above.\n+ * timeout_ms is -1, retry indefinitely. The flags argument, error\n+ * handling, and mode are described above.\n */\n-int hold_lock_file_for_update_timeout(\n+int hold_lock_file_for_update_timeout_mode(\n \t\tstruct lock_file *lk, const char *path,\n-\t\tint flags, long timeout_ms);\n+\t\tint flags, long timeout_ms, int mode);\n+\n+static inline int hold_lock_file_for_update_timeout(\n+\t\tstruct lock_file *lk, const char *path,\n+\t\tint flags, long timeout_ms)\n+{\n+\treturn hold_lock_file_for_update_timeout_mode(lk, path, flags,\n+\t\t\t\t\t\t timeout_ms, 0666);\n+}\n \n /*\n * Attempt to create a lockfile for the file at `path` and return a\n@@ -175,6 +192,13 @@ static inline int hold_lock_file_for_update(\n \treturn hold_lock_file_for_update_timeout(lk, path, flags, 0);\n }\n \n+static inline int hold_lock_file_for_update_mode(\n+\t\tstruct lock_file *lk, const char *path,\n+\t\tint flags, int mode)\n+{\n+\treturn hold_lock_file_for_update_timeout_mode(lk, path, flags, 0, mode);\n+}\n+\n /*\n * Return a nonzero value iff `lk` is currently locked.\n */\n-- \n2.26.0.113.ge9739cdccc\n\n"},{"id":"396582","messageId":"8d5503d2e6723ed5c31a047a28cdc43f7b718cca.1588181626.git.me@ttaylorr.com","threadId":"53267","inReplyTo":"cover.1588181626.git.me@ttaylorr.com","subject":"[PATCH v3 3/5] commit-graph.c: write non-split graphs as read-only","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-04-29T17:36:38Z","receivedAt":"2020-04-29T17:36:43Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"In the previous commit, Git learned 'hold_lock_file_for_update_mode' to\nallow the caller to specify the permission bits (prior to further\nadjustment by the umask and shared repository permissions) used when\nacquiring a temporary file.\n\nUse this in the commit-graph machinery for writing a non-split graph to\nacquire an opened temporary file with permissions read-only permissions\nto match the split behavior. (In the split case, Git uses\ngit_mkstemp_mode' for each of the commit-graph layers with permission\nbits '0444').\n\nOne can notice this discrepancy when moving a non-split graph to be part\nof a new chain. This causes a commit-graph chain where all layers have\nread-only permission bits, except for the base layer, which is writable\nfor the current user.\n\nResolve this discrepancy by using the new\n'hold_lock_file_for_update_mode' and passing the desired permission\nbits.\n\nDoing so causes some test fallout in t5318 and t6600. In t5318, this\noccurs in tests that corrupt a commit-graph file by writing into it. For\nthese, 'chmod u+w'-ing the file beforehand resolves the issue. The\nadditional spot in 'corrupt_graph_verify' is necessary because of the\nextra 'git commit-graph write' beforehand (which *does* rewrite the\ncommit-graph file). In t6600, this is caused by copying a read-only\ncommit-graph file into place and then trying to replace it. For these,\nmake these files writable.\n\nHelped-by: Junio C Hamano \nSigned-off-by: Taylor Blau \n---\n commit-graph.c | 3 ++-\n t/t5318-commit-graph.sh | 15 ++++++++++++++-\n t/t6600-test-reach.sh | 2 ++\n 3 files changed, 18 insertions(+), 2 deletions(-)\n\ndiff --git a/commit-graph.c b/commit-graph.c\nindex f013a84e29..5b5047a7dd 100644\n--- a/commit-graph.c\n+++ b/commit-graph.c\n@@ -1388,7 +1388,8 @@ static int write_commit_graph_file(struct write_commit_graph_context *ctx)\n \n \t\tf = hashfd(fd, ctx->graph_name);\n \t} else {\n-\t\thold_lock_file_for_update(&lk, ctx->graph_name, LOCK_DIE_ON_ERROR);\n+\t\thold_lock_file_for_update_mode(&lk, ctx->graph_name,\n+\t\t\t\t\t LOCK_DIE_ON_ERROR, 0444);\n \t\tfd = lk.tempfile->fd;\n \t\tf = hashfd(lk.tempfile->fd, lk.tempfile->filename.buf);\n \t}\ndiff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh\nindex 9bf920ae17..901eb3ecfb 100755\n--- a/t/t5318-commit-graph.sh\n+++ b/t/t5318-commit-graph.sh\n@@ -12,6 +12,10 @@ test_expect_success 'setup full repo' '\n \ttest_oid_init\n '\n \n+test_expect_success POSIXPERM 'tweak umask for modebit tests' '\n+\tumask 022\n+'\n+\n test_expect_success 'verify graph with no graph file' '\n \tcd \"$TRASH_DIRECTORY/full\" &&\n \tgit commit-graph verify\n@@ -96,6 +100,13 @@ test_expect_success 'write graph' '\n \tgraph_read_expect \"3\"\n '\n \n+test_expect_success POSIXPERM 'write graph has correct permissions' '\n+\ttest_path_is_file $objdir/info/commit-graph &&\n+\techo \"-r--r--r--\" >expect &&\n+\ttest_modebits $objdir/info/commit-graph >actual &&\n+\ttest_cmp expect actual\n+'\n+\n graph_git_behavior 'graph exists' full commits/3 commits/1\n \n test_expect_success 'Add more commits' '\n@@ -421,7 +432,8 @@ GRAPH_BYTE_FOOTER=$(($GRAPH_OCTOPUS_DATA_OFFSET + 4 * $NUM_OCTOPUS_EDGES))\n corrupt_graph_setup() {\n \tcd \"$TRASH_DIRECTORY/full\" &&\n \ttest_when_finished mv commit-graph-backup $objdir/info/commit-graph &&\n-\tcp $objdir/info/commit-graph commit-graph-backup\n+\tcp $objdir/info/commit-graph commit-graph-backup &&\n+\tchmod u+w $objdir/info/commit-graph\n }\n \n corrupt_graph_verify() {\n@@ -435,6 +447,7 @@ corrupt_graph_verify() {\n \tfi &&\n \tgit status --short &&\n \tGIT_TEST_COMMIT_GRAPH_DIE_ON_LOAD=true git commit-graph write &&\n+\tchmod u+w $objdir/info/commit-graph &&\n \tgit commit-graph verify\n }\n \ndiff --git a/t/t6600-test-reach.sh b/t/t6600-test-reach.sh\nindex b24d850036..475564bee7 100755\n--- a/t/t6600-test-reach.sh\n+++ b/t/t6600-test-reach.sh\n@@ -51,8 +51,10 @@ test_expect_success 'setup' '\n \tdone &&\n \tgit commit-graph write --reachable &&\n \tmv .git/objects/info/commit-graph commit-graph-full &&\n+\tchmod u+w commit-graph-full &&\n \tgit show-ref -s commit-5-5 | git commit-graph write --stdin-commits &&\n \tmv .git/objects/info/commit-graph commit-graph-half &&\n+\tchmod u+w commit-graph-half &&\n \tgit config core.commitGraph true\n '\n \n-- \n2.26.0.113.ge9739cdccc\n\n"},{"id":"396583","messageId":"4b74e23af2ac9e65449e383161f7b7cdabf983ba.1588181626.git.me@ttaylorr.com","threadId":"53267","inReplyTo":"cover.1588181626.git.me@ttaylorr.com","subject":"[PATCH v3 4/5] commit-graph.c: ensure graph layers respect core.sharedRepository","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-04-29T17:36:42Z","receivedAt":"2020-04-29T17:36:47Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"Non-layered commit-graphs use 'adjust_shared_perm' to make the\ncommit-graph file readable (or not) to a combination of the user, group,\nand others.\n\nCall 'adjust_shared_perm' for split-graph layers to make sure that these\nalso respect 'core.sharedRepository'. The 'commit-graph-chain' file\nalready respects this configuration since it uses\n'hold_lock_file_for_update' (which calls 'adjust_shared_perm' eventually\nin 'create_tempfile_mode').\n\nSuggested-by: Junio C Hamano \nSigned-off-by: Taylor Blau \n---\n commit-graph.c | 6 ++++++\n t/t5324-split-commit-graph.sh | 22 ++++++++++++++++++++++\n 2 files changed, 28 insertions(+)\n\ndiff --git a/commit-graph.c b/commit-graph.c\nindex 5b5047a7dd..d05a55901d 100644\n--- a/commit-graph.c\n+++ b/commit-graph.c\n@@ -1386,6 +1386,12 @@ static int write_commit_graph_file(struct write_commit_graph_context *ctx)\n \t\t\treturn -1;\n \t\t}\n \n+\t\tif (adjust_shared_perm(ctx->graph_name)) {\n+\t\t\terror(_(\"unable to adjust shared permissions for '%s'\"),\n+\t\t\t ctx->graph_name);\n+\t\t\treturn -1;\n+\t\t}\n+\n \t\tf = hashfd(fd, ctx->graph_name);\n \t} else {\n \t\thold_lock_file_for_update_mode(&lk, ctx->graph_name,\ndiff --git a/t/t5324-split-commit-graph.sh b/t/t5324-split-commit-graph.sh\nindex 53b2e6b455..699c23d077 100755\n--- a/t/t5324-split-commit-graph.sh\n+++ b/t/t5324-split-commit-graph.sh\n@@ -36,6 +36,10 @@ graph_read_expect() {\n \ttest_cmp expect output\n }\n \n+test_expect_success POSIXPERM 'tweak umask for modebit tests' '\n+\tumask 022\n+'\n+\n test_expect_success 'create commits and write commit-graph' '\n \tfor i in $(test_seq 3)\n \tdo\n@@ -351,4 +355,22 @@ test_expect_success 'split across alternate where alternate is not split' '\n \ttest_cmp commit-graph .git/objects/info/commit-graph\n '\n \n+while read mode modebits\n+do\n+\ttest_expect_success POSIXPERM \"split commit-graph respects core.sharedrepository $mode\" '\n+\t\trm -rf $graphdir $infodir/commit-graph &&\n+\t\tgit reset --hard commits/1 &&\n+\t\ttest_config core.sharedrepository \"$mode\" &&\n+\t\tgit commit-graph write --split --reachable &&\n+\t\tls $graphdir/graph-*.graph >graph-files &&\n+\t\ttest_line_count = 1 graph-files &&\n+\t\techo \"$modebits\" >expect &&\n+\t\ttest_modebits $graphdir/graph-*.graph >actual &&\n+\t\ttest_cmp expect actual\n+\t'\n+done <<\\EOF\n+0666 -r--r--r--\n+0600 -r--------\n+EOF\n+\n test_done\n-- \n2.26.0.113.ge9739cdccc\n\n"},{"id":"396584","messageId":"864c9160675ba199b140ba961258db8d0daad004.1588181626.git.me@ttaylorr.com","threadId":"53267","inReplyTo":"cover.1588181626.git.me@ttaylorr.com","subject":"[PATCH v3 5/5] commit-graph.c: make 'commit-graph-chain's read-only","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-04-29T17:36:46Z","receivedAt":"2020-04-29T17:36:50Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"In a previous commit, we made incremental graph layers read-only by\nusing 'git_mkstemp_mode' with permissions '0444'.\n\nThere is no reason that 'commit-graph-chain's should be modifiable by\nthe user, since they are generated at a temporary location and then\natomically renamed into place.\n\nTo ensure that these files are read-only, too, use\n'hold_lock_file_for_update_mode' with the same read-only permission\nbits, and let the umask and 'adjust_shared_perm' take care of the rest.\n\nSigned-off-by: Taylor Blau \n---\n commit-graph.c | 3 ++-\n t/t5324-split-commit-graph.sh | 2 ++\n 2 files changed, 4 insertions(+), 1 deletion(-)\n\ndiff --git a/commit-graph.c b/commit-graph.c\nindex d05a55901d..b2dfd7701f 100644\n--- a/commit-graph.c\n+++ b/commit-graph.c\n@@ -1378,7 +1378,8 @@ static int write_commit_graph_file(struct write_commit_graph_context *ctx)\n \tif (ctx->split) {\n \t\tchar *lock_name = get_chain_filename(ctx->odb);\n \n-\t\thold_lock_file_for_update(&lk, lock_name, LOCK_DIE_ON_ERROR);\n+\t\thold_lock_file_for_update_mode(&lk, lock_name,\n+\t\t\t\t\t LOCK_DIE_ON_ERROR, 0444);\n \n \t\tfd = git_mkstemp_mode(ctx->graph_name, 0444);\n \t\tif (fd < 0) {\ndiff --git a/t/t5324-split-commit-graph.sh b/t/t5324-split-commit-graph.sh\nindex 699c23d077..cff5a41f48 100755\n--- a/t/t5324-split-commit-graph.sh\n+++ b/t/t5324-split-commit-graph.sh\n@@ -366,6 +366,8 @@ do\n \t\ttest_line_count = 1 graph-files &&\n \t\techo \"$modebits\" >expect &&\n \t\ttest_modebits $graphdir/graph-*.graph >actual &&\n+\t\ttest_cmp expect actual &&\n+\t\ttest_modebits $graphdir/commit-graph-chain >actual &&\n \t\ttest_cmp expect actual\n \t'\n done <<\\EOF\n-- \n2.26.0.113.ge9739cdccc\n"},{"id":"396715","messageId":"20200501055225.GA23665@coredump.intra.peff.net","threadId":"53267","inReplyTo":"cover.1588181626.git.me@ttaylorr.com","subject":"Re: [PATCH v3 0/5] commit-graph: write non-split graphs as read-only","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-05-01T05:52:25Z","receivedAt":"2020-05-01T05:59:49Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Apr 29, 2020 at 11:36:26AM -0600, Taylor Blau wrote:\n\n> Here's a brief reroll to incorporate suggestions from Junio and Peff in\n> the sub-thread beginning at [1]. Not much at all has changed since v2,\n> with the exception of:\n> \n> * fixing the aforementioned issue by sprinkling in some 'umask 022's\n> in throughout the new tests\n> \n> * adding in a new final patch to apply the same treatment to\n> the 'commit-graph-chain' file\n\nThanks, this version looks good to me.\n\n-Peff\n"}]}