{"thread":{"id":"59665","subject":"[PATCH] setup: trace bare repository setups","startedAt":"2023-04-27T22:32:51Z","lastAt":"2023-05-10T23:29:17Z","messageCount":16,"participants":["Josh Steadmon","Junio C Hamano","Glen Choo","Taylor Blau"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"476243","messageId":"cb72bca46c6ff2a8cf3196408fb53411f7f91892.1682631601.git.steadmon@google.com","threadId":"59665","inReplyTo":null,"subject":"[PATCH] setup: trace bare repository setups","fromName":"Josh Steadmon","fromEmail":"steadmon@google.com","sentAt":"2023-04-27T22:32:40Z","receivedAt":"2023-04-27T22:32:51Z","isPatch":true,"sender":{"key":"steadmon@google.com","avatar":"https://avatars.githubusercontent.com/u/2654920?v=4"},"body":"From: Glen Choo <chooglen@google.com>\n\nsafe.bareRepository=explicit is a safer default mode of operation, since\nit guards against the embedded bare repository attack [1]. Most end\nusers don't use bare repositories directly, so they should be able to\nset safe.bareRepository=explicit, with the expectation that they can\nreenable bare repositories by specifying GIT_DIR or --git-dir.\n\nHowever, the user might use a tool that invokes Git on bare repositories\nwithout setting GIT_DIR (e.g. \"go mod\" will clone bare repositories\n[2]), so even if a user wanted to use safe.bareRepository=explicit, it\nwouldn't be feasible until their tools learned to set GIT_DIR.\n\nTo make this transition easier, add a trace message to note when we\nattempt to set up a bare repository without setting GIT_DIR. This allows\nusers and tool developers to audit which of their tools are problematic\nand report/fix the issue.  When they are sufficiently confident, they\nwould switch over to \"safe.bareRepository=explicit\".\n\nNote that this uses trace2_data_string(), which isn't supported by the\n\"normal\" GIT_TRACE2 target, only _EVENT or _PERF.\n\n[1] https://lore.kernel.org/git/kl6lsfqpygsj.fsf@chooglen-macbookpro.roam.corp.google.com/\n[2] https://go.dev/ref/mod\n\nSigned-off-by: Glen Choo <chooglen@google.com>\nSigned-off-by: Josh Steadmon <steadmon@google.com>\nChange-Id: I8e8b5e70ce8c6c81ec4716187c27c44da38b35db\n---\n setup.c                         |  1 +\n t/t0035-safe-bare-repository.sh | 29 ++++++++++++++++++++++-------\n 2 files changed, 23 insertions(+), 7 deletions(-)\n\ndiff --git a/setup.c b/setup.c\nindex 59abc16ba6..458582207e 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -1352,6 +1352,7 @@ static enum discovery_result setup_git_directory_gently_1(struct strbuf *dir,\n \t\t}\n \n \t\tif (is_git_directory(dir->buf)) {\n+\t\t\ttrace2_data_string(\"setup\", NULL, \"implicit-bare-repository\", dir->buf);\n \t\t\tif (get_allowed_bare_repo() == ALLOWED_BARE_REPO_EXPLICIT)\n \t\t\t\treturn GIT_DIR_DISALLOWED_BARE;\n \t\t\tif (!ensure_valid_ownership(NULL, NULL, dir->buf, report))\ndiff --git a/t/t0035-safe-bare-repository.sh b/t/t0035-safe-bare-repository.sh\nindex 11c15a48aa..a1f3b5a4d6 100755\n--- a/t/t0035-safe-bare-repository.sh\n+++ b/t/t0035-safe-bare-repository.sh\n@@ -7,13 +7,24 @@ TEST_PASSES_SANITIZE_LEAK=true\n \n pwd=\"$(pwd)\"\n \n-expect_accepted () {\n-\tgit \"$@\" rev-parse --git-dir\n+expect_accepted_implicit () {\n+\ttest_when_finished \"rm \\\"$pwd/trace.perf\\\"\" &&\n+\tGIT_TRACE2_PERF=\"$pwd/trace.perf\" git \"$@\" rev-parse --git-dir &&\n+\tgrep -F \"implicit-bare-repository:$pwd\" \"$pwd/trace.perf\"\n+}\n+\n+expect_accepted_explicit () {\n+\ttest_when_finished \"rm \\\"$pwd/trace.perf\\\"\" &&\n+\tGIT_DIR=\"$@\" GIT_TRACE2_PERF=\"$pwd/trace.perf\" git rev-parse --git-dir &&\n+\t! grep -F \"implicit-bare-repository:$pwd\" \"$pwd/trace.perf\"\n }\n \n expect_rejected () {\n-\ttest_must_fail git \"$@\" rev-parse --git-dir 2>err &&\n-\tgrep -F \"cannot use bare repository\" err\n+\ttest_when_finished \"rm \\\"$pwd/trace.perf\\\"\" &&\n+\ttest_env GIT_TRACE2_PERF=\"$pwd/trace.perf\" \\\n+\t\ttest_must_fail git \"$@\" rev-parse --git-dir 2>err &&\n+\tgrep -F \"cannot use bare repository\" err &&\n+\tgrep -F \"implicit-bare-repository:$pwd\" \"$pwd/trace.perf\"\n }\n \n test_expect_success 'setup bare repo in worktree' '\n@@ -22,12 +33,12 @@ test_expect_success 'setup bare repo in worktree' '\n '\n \n test_expect_success 'safe.bareRepository unset' '\n-\texpect_accepted -C outer-repo/bare-repo\n+\texpect_accepted_implicit -C outer-repo/bare-repo\n '\n \n test_expect_success 'safe.bareRepository=all' '\n \ttest_config_global safe.bareRepository all &&\n-\texpect_accepted -C outer-repo/bare-repo\n+\texpect_accepted_implicit -C outer-repo/bare-repo\n '\n \n test_expect_success 'safe.bareRepository=explicit' '\n@@ -47,7 +58,7 @@ test_expect_success 'safe.bareRepository in the repository' '\n \n test_expect_success 'safe.bareRepository on the command line' '\n \ttest_config_global safe.bareRepository explicit &&\n-\texpect_accepted -C outer-repo/bare-repo \\\n+\texpect_accepted_implicit -C outer-repo/bare-repo \\\n \t\t-c safe.bareRepository=all\n '\n \n@@ -60,4 +71,8 @@ test_expect_success 'safe.bareRepository in included file' '\n \texpect_rejected -C outer-repo/bare-repo\n '\n \n+test_expect_success 'no trace when GIT_DIR is explicitly provided' '\n+\texpect_accepted_explicit \"$pwd/outer-repo/bare-repo\"\n+'\n+\n test_done\n\nbase-commit: 2807bd2c10606e590886543afe4e4f208dddd489\n-- \n2.40.1.495.gc816e09b53d-goog\n\n"},{"id":"476247","messageId":"xmqqttx1gcmr.fsf@gitster.g","threadId":"59665","inReplyTo":"cb72bca46c6ff2a8cf3196408fb53411f7f91892.1682631601.git.steadmon@google.com","subject":"Re: [PATCH] setup: trace bare repository setups","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-04-27T22:54:04Z","receivedAt":"2023-04-27T22:54:10Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Josh Steadmon <steadmon@google.com> writes:\n\n> diff --git a/setup.c b/setup.c\n> index 59abc16ba6..458582207e 100644\n> --- a/setup.c\n> +++ b/setup.c\n> @@ -1352,6 +1352,7 @@ static enum discovery_result setup_git_directory_gently_1(struct strbuf *dir,\n>  \t\t}\n>  \n>  \t\tif (is_git_directory(dir->buf)) {\n> +\t\t\ttrace2_data_string(\"setup\", NULL, \"implicit-bare-repository\", dir->buf);\n>  \t\t\tif (get_allowed_bare_repo() == ALLOWED_BARE_REPO_EXPLICIT)\n>  \t\t\t\treturn GIT_DIR_DISALLOWED_BARE;\n>  \t\t\tif (!ensure_valid_ownership(NULL, NULL, dir->buf, report))\n\nThat is kind of obvious.\n\n> diff --git a/t/t0035-safe-bare-repository.sh b/t/t0035-safe-bare-repository.sh\n> index 11c15a48aa..a1f3b5a4d6 100755\n> --- a/t/t0035-safe-bare-repository.sh\n> +++ b/t/t0035-safe-bare-repository.sh\n> @@ -7,13 +7,24 @@ TEST_PASSES_SANITIZE_LEAK=true\n>  \n>  pwd=\"$(pwd)\"\n>  \n> -expect_accepted () {\n> -\tgit \"$@\" rev-parse --git-dir\n> +expect_accepted_implicit () {\n> +\ttest_when_finished \"rm \\\"$pwd/trace.perf\\\"\" &&\n\nWhy not\n\n\ttest_when_finished 'rm \"$pwd/trace.perf\"' &&\n\ninstead?  \n\nIn your version, $pwd is expanded before test_when_finished sees it,\nso you'd have to worry about things like backslashes and double quotes\nin it.  You can of course quote the '$' like so\n\n\ttest_when_finished \"rm \\\"\\$pwd/trace.perf\\\"\" &&\n\nto work it around, but it is equivalent to enclosing everything\ninside a pair of single quotes.  Either way your $pwd will be\ninterpolated when \"eval\" sees the $test_cleanup script.\n\nAnd it would be much easier to read without backslash and\nbackslashed double quotes.\n\n> +\tGIT_TRACE2_PERF=\"$pwd/trace.perf\" git \"$@\" rev-parse --git-dir &&\n> +\tgrep -F \"implicit-bare-repository:$pwd\" \"$pwd/trace.perf\"\n> +}\n\nWe ensure that we positively have such a trace in the output.  Good.\n\n> +expect_accepted_explicit () {\n> +\ttest_when_finished \"rm \\\"$pwd/trace.perf\\\"\" &&\n> +\tGIT_DIR=\"$@\" GIT_TRACE2_PERF=\"$pwd/trace.perf\" git rev-parse --git-dir &&\n> +\t! grep -F \"implicit-bare-repository:$pwd\" \"$pwd/trace.perf\"\n>  }\n\nWhen not asking for the magic behaviour of \"$@\" and instead doing a\n\"squash everything into a single string, using the first letter in\n$IFS as the separator in between\", please write \"$*\" instead.\n\n    GIT_DIR=\"$*\" GIT_TRACE2_PERF=\"...\" git rev-parse --git-dir\n\nBut in this case, I do not think you are ever planning to send a\ndirectory name split into two or more, to be concatenated with SP,\nso writing it like\n\n    GIT_DIR=\"$1\" GIT_TRACE2_PERF=\"...\" git rev-parse --git-dir\n\nwould be much less error prone and easier to follow, I think.\n\n> @@ -22,12 +33,12 @@ test_expect_success 'setup bare repo in worktree' '\n>  '\n>  \n>  test_expect_success 'safe.bareRepository unset' '\n> -\texpect_accepted -C outer-repo/bare-repo\n> +\texpect_accepted_implicit -C outer-repo/bare-repo\n>  '\n\nPerhaps futureproof this test piece by explicitly unsetting the\nvariable before starting the test?  That way, this piece will not be\nbroken even if earlier tests gets modified to set some value to\nsafe.bareRepository in the future.\n\n>  test_expect_success 'safe.bareRepository=all' '\n>  \ttest_config_global safe.bareRepository all &&\n> -\texpect_accepted -C outer-repo/bare-repo\n> +\texpect_accepted_implicit -C outer-repo/bare-repo\n>  '\n>  \n>  test_expect_success 'safe.bareRepository=explicit' '\n> @@ -47,7 +58,7 @@ test_expect_success 'safe.bareRepository in the repository' '\n>  \n>  test_expect_success 'safe.bareRepository on the command line' '\n>  \ttest_config_global safe.bareRepository explicit &&\n> -\texpect_accepted -C outer-repo/bare-repo \\\n> +\texpect_accepted_implicit -C outer-repo/bare-repo \\\n>  \t\t-c safe.bareRepository=all\n>  '\n>  \n> @@ -60,4 +71,8 @@ test_expect_success 'safe.bareRepository in included file' '\n>  \texpect_rejected -C outer-repo/bare-repo\n>  '\n>  \n> +test_expect_success 'no trace when GIT_DIR is explicitly provided' '\n> +\texpect_accepted_explicit \"$pwd/outer-repo/bare-repo\"\n> +'\n> +\n>  test_done\n\nAll the expectations look sensible.  Thanks for a pleasant read.\n"},{"id":"476251","messageId":"kl6lcz3onbhx.fsf@chooglen-macbookpro.roam.corp.google.com","threadId":"59665","inReplyTo":"cb72bca46c6ff2a8cf3196408fb53411f7f91892.1682631601.git.steadmon@google.com","subject":"Re: [PATCH] setup: trace bare repository setups","fromName":"Glen Choo","fromEmail":"chooglen@google.com","sentAt":"2023-04-27T23:36:42Z","receivedAt":"2023-04-27T23:37:34Z","isPatch":true,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"Josh Steadmon <steadmon@google.com> writes:\n\n> From: Glen Choo <chooglen@google.com>\n\nI can confirm that I did write an initial version of this, which Josh\ncleaned up for the mailing list (thanks!). Most, if not all, of the\nmistakes are originally mine.\n\n> To make this transition easier, add a trace message to note when we\n> attempt to set up a bare repository without setting GIT_DIR. This allows\n> users and tool developers to audit which of their tools are problematic\n> and report/fix the issue.  When they are sufficiently confident, they\n> would switch over to \"safe.bareRepository=explicit\".\n\nOne alternative to this is to trace all of the repository setup process.\nE.g. if we traced the data points in t/t1510-repo-setup.sh, like GIT_DIR\nand whether the repository is bare, you could reverse-engineer whether\nwe've hit the \"set up a bare repository without GIT_DIR\" case, but\nthat's significantly more complicated. If the goal of this patch is to\nmake it easy for users, tool developers and sysadmins to see if\n\"safe.bareRepository=explicit\" might be tripped, giving a single,\nmeaningful event is much easier way to get there.\n\nIt would be nice to trace all of the repo setup eventually, anyway, and\nI don't think this change precludes that.\n\n> Change-Id: I8e8b5e70ce8c6c81ec4716187c27c44da38b35db\n\nLeftover from Gerrit, perhaps?\n\nUnsurprisingly, I don't have comments on the diff, at least not anything\nthat Junio hasn't already spotted.\n"},{"id":"476274","messageId":"ZEv38M7v3tROT6Nw@google.com","threadId":"59665","inReplyTo":"kl6lcz3onbhx.fsf@chooglen-macbookpro.roam.corp.google.com","subject":"Re: [PATCH] setup: trace bare repository setups","fromName":"Josh Steadmon","fromEmail":"steadmon@google.com","sentAt":"2023-04-28T16:48:33Z","receivedAt":"2023-04-28T16:48:53Z","isPatch":true,"sender":{"key":"steadmon@google.com","avatar":"https://avatars.githubusercontent.com/u/2654920?v=4"},"body":"On 2023.04.27 16:36, Glen Choo wrote:\n> Josh Steadmon <steadmon@google.com> writes:\n> \n> > From: Glen Choo <chooglen@google.com>\n> \n> I can confirm that I did write an initial version of this, which Josh\n> cleaned up for the mailing list (thanks!). Most, if not all, of the\n> mistakes are originally mine.\n> \n> > To make this transition easier, add a trace message to note when we\n> > attempt to set up a bare repository without setting GIT_DIR. This allows\n> > users and tool developers to audit which of their tools are problematic\n> > and report/fix the issue.  When they are sufficiently confident, they\n> > would switch over to \"safe.bareRepository=explicit\".\n> \n> One alternative to this is to trace all of the repository setup process.\n> E.g. if we traced the data points in t/t1510-repo-setup.sh, like GIT_DIR\n> and whether the repository is bare, you could reverse-engineer whether\n> we've hit the \"set up a bare repository without GIT_DIR\" case, but\n> that's significantly more complicated. If the goal of this patch is to\n> make it easy for users, tool developers and sysadmins to see if\n> \"safe.bareRepository=explicit\" might be tripped, giving a single,\n> meaningful event is much easier way to get there.\n> \n> It would be nice to trace all of the repo setup eventually, anyway, and\n> I don't think this change precludes that.\n> \n> > Change-Id: I8e8b5e70ce8c6c81ec4716187c27c44da38b35db\n> \n> Leftover from Gerrit, perhaps?\n> \n> Unsurprisingly, I don't have comments on the diff, at least not anything\n> that Junio hasn't already spotted.\n\nWhoops, thanks for the catch. It would seem my send-email workflow broke\nsomehow while I was away from work. I also had a comment explaining why\nI was sending this out for you, which somehow got dropped.\n"},{"id":"476275","messageId":"ZEv6LZTx1pZpJtIn@google.com","threadId":"59665","inReplyTo":"xmqqttx1gcmr.fsf@gitster.g","subject":"Re: [PATCH] setup: trace bare repository setups","fromName":"Josh Steadmon","fromEmail":"steadmon@google.com","sentAt":"2023-04-28T16:54:05Z","receivedAt":"2023-04-28T16:54:25Z","isPatch":true,"sender":{"key":"steadmon@google.com","avatar":"https://avatars.githubusercontent.com/u/2654920?v=4"},"body":"On 2023.04.27 15:54, Junio C Hamano wrote:\n> Josh Steadmon <steadmon@google.com> writes:\n> \n> > diff --git a/setup.c b/setup.c\n> > index 59abc16ba6..458582207e 100644\n> > --- a/setup.c\n> > +++ b/setup.c\n> > @@ -1352,6 +1352,7 @@ static enum discovery_result setup_git_directory_gently_1(struct strbuf *dir,\n> >  \t\t}\n> >  \n> >  \t\tif (is_git_directory(dir->buf)) {\n> > +\t\t\ttrace2_data_string(\"setup\", NULL, \"implicit-bare-repository\", dir->buf);\n> >  \t\t\tif (get_allowed_bare_repo() == ALLOWED_BARE_REPO_EXPLICIT)\n> >  \t\t\t\treturn GIT_DIR_DISALLOWED_BARE;\n> >  \t\t\tif (!ensure_valid_ownership(NULL, NULL, dir->buf, report))\n> \n> That is kind of obvious.\n> \n> > diff --git a/t/t0035-safe-bare-repository.sh b/t/t0035-safe-bare-repository.sh\n> > index 11c15a48aa..a1f3b5a4d6 100755\n> > --- a/t/t0035-safe-bare-repository.sh\n> > +++ b/t/t0035-safe-bare-repository.sh\n> > @@ -7,13 +7,24 @@ TEST_PASSES_SANITIZE_LEAK=true\n> >  \n> >  pwd=\"$(pwd)\"\n> >  \n> > -expect_accepted () {\n> > -\tgit \"$@\" rev-parse --git-dir\n> > +expect_accepted_implicit () {\n> > +\ttest_when_finished \"rm \\\"$pwd/trace.perf\\\"\" &&\n> \n> Why not\n> \n> \ttest_when_finished 'rm \"$pwd/trace.perf\"' &&\n> \n> instead?  \n> \n> In your version, $pwd is expanded before test_when_finished sees it,\n> so you'd have to worry about things like backslashes and double quotes\n> in it.  You can of course quote the '$' like so\n> \n> \ttest_when_finished \"rm \\\"\\$pwd/trace.perf\\\"\" &&\n> \n> to work it around, but it is equivalent to enclosing everything\n> inside a pair of single quotes.  Either way your $pwd will be\n> interpolated when \"eval\" sees the $test_cleanup script.\n> \n> And it would be much easier to read without backslash and\n> backslashed double quotes.\n> \n> > +\tGIT_TRACE2_PERF=\"$pwd/trace.perf\" git \"$@\" rev-parse --git-dir &&\n> > +\tgrep -F \"implicit-bare-repository:$pwd\" \"$pwd/trace.perf\"\n> > +}\n> \n> We ensure that we positively have such a trace in the output.  Good.\n> \n> > +expect_accepted_explicit () {\n> > +\ttest_when_finished \"rm \\\"$pwd/trace.perf\\\"\" &&\n> > +\tGIT_DIR=\"$@\" GIT_TRACE2_PERF=\"$pwd/trace.perf\" git rev-parse --git-dir &&\n> > +\t! grep -F \"implicit-bare-repository:$pwd\" \"$pwd/trace.perf\"\n> >  }\n> \n> When not asking for the magic behaviour of \"$@\" and instead doing a\n> \"squash everything into a single string, using the first letter in\n> $IFS as the separator in between\", please write \"$*\" instead.\n> \n>     GIT_DIR=\"$*\" GIT_TRACE2_PERF=\"...\" git rev-parse --git-dir\n> \n> But in this case, I do not think you are ever planning to send a\n> directory name split into two or more, to be concatenated with SP,\n> so writing it like\n> \n>     GIT_DIR=\"$1\" GIT_TRACE2_PERF=\"...\" git rev-parse --git-dir\n> \n> would be much less error prone and easier to follow, I think.\n> \n> > @@ -22,12 +33,12 @@ test_expect_success 'setup bare repo in worktree' '\n> >  '\n> >  \n> >  test_expect_success 'safe.bareRepository unset' '\n> > -\texpect_accepted -C outer-repo/bare-repo\n> > +\texpect_accepted_implicit -C outer-repo/bare-repo\n> >  '\n> \n> Perhaps futureproof this test piece by explicitly unsetting the\n> variable before starting the test?  That way, this piece will not be\n> broken even if earlier tests gets modified to set some value to\n> safe.bareRepository in the future.\n> \n> >  test_expect_success 'safe.bareRepository=all' '\n> >  \ttest_config_global safe.bareRepository all &&\n> > -\texpect_accepted -C outer-repo/bare-repo\n> > +\texpect_accepted_implicit -C outer-repo/bare-repo\n> >  '\n> >  \n> >  test_expect_success 'safe.bareRepository=explicit' '\n> > @@ -47,7 +58,7 @@ test_expect_success 'safe.bareRepository in the repository' '\n> >  \n> >  test_expect_success 'safe.bareRepository on the command line' '\n> >  \ttest_config_global safe.bareRepository explicit &&\n> > -\texpect_accepted -C outer-repo/bare-repo \\\n> > +\texpect_accepted_implicit -C outer-repo/bare-repo \\\n> >  \t\t-c safe.bareRepository=all\n> >  '\n> >  \n> > @@ -60,4 +71,8 @@ test_expect_success 'safe.bareRepository in included file' '\n> >  \texpect_rejected -C outer-repo/bare-repo\n> >  '\n> >  \n> > +test_expect_success 'no trace when GIT_DIR is explicitly provided' '\n> > +\texpect_accepted_explicit \"$pwd/outer-repo/bare-repo\"\n> > +'\n> > +\n> >  test_done\n> \n> All the expectations look sensible.  Thanks for a pleasant read.\n\nAgreed with all the feedback points, will fix in V2. Thanks!\n"},{"id":"476276","messageId":"ZEv8AcxQW7B2H8uJ@google.com","threadId":"59665","inReplyTo":"xmqqttx1gcmr.fsf@gitster.g","subject":"Re: [PATCH] setup: trace bare repository setups","fromName":"Josh Steadmon","fromEmail":"steadmon@google.com","sentAt":"2023-04-28T17:01:53Z","receivedAt":"2023-04-28T17:02:23Z","isPatch":true,"sender":{"key":"steadmon@google.com","avatar":"https://avatars.githubusercontent.com/u/2654920?v=4"},"body":"On 2023.04.27 15:54, Junio C Hamano wrote:\n> Josh Steadmon <steadmon@google.com> writes:\n> \n> > @@ -22,12 +33,12 @@ test_expect_success 'setup bare repo in worktree' '\n> >  '\n> >  \n> >  test_expect_success 'safe.bareRepository unset' '\n> > -\texpect_accepted -C outer-repo/bare-repo\n> > +\texpect_accepted_implicit -C outer-repo/bare-repo\n> >  '\n> \n> Perhaps futureproof this test piece by explicitly unsetting the\n> variable before starting the test?  That way, this piece will not be\n> broken even if earlier tests gets modified to set some value to\n> safe.bareRepository in the future.\n\nActually, explicitly setting the variable here is equivalent to the\nfollowing test case, so I'll just remove this one.\n"},{"id":"476278","messageId":"b548d96db7052dc2cb664922fa2003fe068843cf.1682702058.git.steadmon@google.com","threadId":"59665","inReplyTo":"cb72bca46c6ff2a8cf3196408fb53411f7f91892.1682631601.git.steadmon@google.com","subject":"[PATCH v2] setup: trace bare repository setups","fromName":"Josh Steadmon","fromEmail":"steadmon@google.com","sentAt":"2023-04-28T17:22:10Z","receivedAt":"2023-04-28T17:22:16Z","isPatch":true,"sender":{"key":"steadmon@google.com","avatar":"https://avatars.githubusercontent.com/u/2654920?v=4"},"body":"From: Glen Choo <chooglen@google.com>\n\nsafe.bareRepository=explicit is a safer default mode of operation, since\nit guards against the embedded bare repository attack [1]. Most end\nusers don't use bare repositories directly, so they should be able to\nset safe.bareRepository=explicit, with the expectation that they can\nreenable bare repositories by specifying GIT_DIR or --git-dir.\n\nHowever, the user might use a tool that invokes Git on bare repositories\nwithout setting GIT_DIR (e.g. \"go mod\" will clone bare repositories\n[2]), so even if a user wanted to use safe.bareRepository=explicit, it\nwouldn't be feasible until their tools learned to set GIT_DIR.\n\nTo make this transition easier, add a trace message to note when we\nattempt to set up a bare repository without setting GIT_DIR. This allows\nusers and tool developers to audit which of their tools are problematic\nand report/fix the issue.  When they are sufficiently confident, they\nwould switch over to \"safe.bareRepository=explicit\".\n\nNote that this uses trace2_data_string(), which isn't supported by the\n\"normal\" GIT_TRACE2 target, only _EVENT or _PERF.\n\n[1] https://lore.kernel.org/git/kl6lsfqpygsj.fsf@chooglen-macbookpro.roam.corp.google.com/\n[2] https://go.dev/ref/mod\n\nSigned-off-by: Glen Choo <chooglen@google.com>\nSigned-off-by: Josh Steadmon <steadmon@google.com>\n---\nI'm sending a lightly-adapted version of Glen's tracing patch because\nGlen will be on vacation next week and we'd like to get this upstream\nASAP.\n\nCleaned up some test-style issues since V1.\n\nRange-diff against v1:\n1:  cb72bca46c ! 1:  b548d96db7 setup: trace bare repository setups\n    @@ t/t0035-safe-bare-repository.sh: TEST_PASSES_SANITIZE_LEAK=true\n     -expect_accepted () {\n     -\tgit \"$@\" rev-parse --git-dir\n     +expect_accepted_implicit () {\n    -+\ttest_when_finished \"rm \\\"$pwd/trace.perf\\\"\" &&\n    ++\ttest_when_finished 'rm \"$pwd/trace.perf\"' &&\n     +\tGIT_TRACE2_PERF=\"$pwd/trace.perf\" git \"$@\" rev-parse --git-dir &&\n     +\tgrep -F \"implicit-bare-repository:$pwd\" \"$pwd/trace.perf\"\n     +}\n     +\n     +expect_accepted_explicit () {\n    -+\ttest_when_finished \"rm \\\"$pwd/trace.perf\\\"\" &&\n    -+\tGIT_DIR=\"$@\" GIT_TRACE2_PERF=\"$pwd/trace.perf\" git rev-parse --git-dir &&\n    ++\ttest_when_finished 'rm \"$pwd/trace.perf\"' &&\n    ++\tGIT_DIR=\"$1\" GIT_TRACE2_PERF=\"$pwd/trace.perf\" git rev-parse --git-dir &&\n     +\t! grep -F \"implicit-bare-repository:$pwd\" \"$pwd/trace.perf\"\n      }\n      \n      expect_rejected () {\n     -\ttest_must_fail git \"$@\" rev-parse --git-dir 2>err &&\n     -\tgrep -F \"cannot use bare repository\" err\n    -+\ttest_when_finished \"rm \\\"$pwd/trace.perf\\\"\" &&\n    ++\ttest_when_finished 'rm \"$pwd/trace.perf\"' &&\n     +\ttest_env GIT_TRACE2_PERF=\"$pwd/trace.perf\" \\\n     +\t\ttest_must_fail git \"$@\" rev-parse --git-dir 2>err &&\n     +\tgrep -F \"cannot use bare repository\" err &&\n    @@ t/t0035-safe-bare-repository.sh: TEST_PASSES_SANITIZE_LEAK=true\n      \n      test_expect_success 'setup bare repo in worktree' '\n     @@ t/t0035-safe-bare-repository.sh: test_expect_success 'setup bare repo in worktree' '\n    + \tgit init --bare outer-repo/bare-repo\n      '\n      \n    - test_expect_success 'safe.bareRepository unset' '\n    +-test_expect_success 'safe.bareRepository unset' '\n     -\texpect_accepted -C outer-repo/bare-repo\n    -+\texpect_accepted_implicit -C outer-repo/bare-repo\n    - '\n    - \n    +-'\n    +-\n      test_expect_success 'safe.bareRepository=all' '\n      \ttest_config_global safe.bareRepository all &&\n     -\texpect_accepted -C outer-repo/bare-repo\n\n setup.c                         |  1 +\n t/t0035-safe-bare-repository.sh | 31 +++++++++++++++++++++----------\n 2 files changed, 22 insertions(+), 10 deletions(-)\n\ndiff --git a/setup.c b/setup.c\nindex 59abc16ba6..458582207e 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -1352,6 +1352,7 @@ static enum discovery_result setup_git_directory_gently_1(struct strbuf *dir,\n \t\t}\n \n \t\tif (is_git_directory(dir->buf)) {\n+\t\t\ttrace2_data_string(\"setup\", NULL, \"implicit-bare-repository\", dir->buf);\n \t\t\tif (get_allowed_bare_repo() == ALLOWED_BARE_REPO_EXPLICIT)\n \t\t\t\treturn GIT_DIR_DISALLOWED_BARE;\n \t\t\tif (!ensure_valid_ownership(NULL, NULL, dir->buf, report))\ndiff --git a/t/t0035-safe-bare-repository.sh b/t/t0035-safe-bare-repository.sh\nindex 11c15a48aa..993f5bdc7d 100755\n--- a/t/t0035-safe-bare-repository.sh\n+++ b/t/t0035-safe-bare-repository.sh\n@@ -7,13 +7,24 @@ TEST_PASSES_SANITIZE_LEAK=true\n \n pwd=\"$(pwd)\"\n \n-expect_accepted () {\n-\tgit \"$@\" rev-parse --git-dir\n+expect_accepted_implicit () {\n+\ttest_when_finished 'rm \"$pwd/trace.perf\"' &&\n+\tGIT_TRACE2_PERF=\"$pwd/trace.perf\" git \"$@\" rev-parse --git-dir &&\n+\tgrep -F \"implicit-bare-repository:$pwd\" \"$pwd/trace.perf\"\n+}\n+\n+expect_accepted_explicit () {\n+\ttest_when_finished 'rm \"$pwd/trace.perf\"' &&\n+\tGIT_DIR=\"$1\" GIT_TRACE2_PERF=\"$pwd/trace.perf\" git rev-parse --git-dir &&\n+\t! grep -F \"implicit-bare-repository:$pwd\" \"$pwd/trace.perf\"\n }\n \n expect_rejected () {\n-\ttest_must_fail git \"$@\" rev-parse --git-dir 2>err &&\n-\tgrep -F \"cannot use bare repository\" err\n+\ttest_when_finished 'rm \"$pwd/trace.perf\"' &&\n+\ttest_env GIT_TRACE2_PERF=\"$pwd/trace.perf\" \\\n+\t\ttest_must_fail git \"$@\" rev-parse --git-dir 2>err &&\n+\tgrep -F \"cannot use bare repository\" err &&\n+\tgrep -F \"implicit-bare-repository:$pwd\" \"$pwd/trace.perf\"\n }\n \n test_expect_success 'setup bare repo in worktree' '\n@@ -21,13 +32,9 @@ test_expect_success 'setup bare repo in worktree' '\n \tgit init --bare outer-repo/bare-repo\n '\n \n-test_expect_success 'safe.bareRepository unset' '\n-\texpect_accepted -C outer-repo/bare-repo\n-'\n-\n test_expect_success 'safe.bareRepository=all' '\n \ttest_config_global safe.bareRepository all &&\n-\texpect_accepted -C outer-repo/bare-repo\n+\texpect_accepted_implicit -C outer-repo/bare-repo\n '\n \n test_expect_success 'safe.bareRepository=explicit' '\n@@ -47,7 +54,7 @@ test_expect_success 'safe.bareRepository in the repository' '\n \n test_expect_success 'safe.bareRepository on the command line' '\n \ttest_config_global safe.bareRepository explicit &&\n-\texpect_accepted -C outer-repo/bare-repo \\\n+\texpect_accepted_implicit -C outer-repo/bare-repo \\\n \t\t-c safe.bareRepository=all\n '\n \n@@ -60,4 +67,8 @@ test_expect_success 'safe.bareRepository in included file' '\n \texpect_rejected -C outer-repo/bare-repo\n '\n \n+test_expect_success 'no trace when GIT_DIR is explicitly provided' '\n+\texpect_accepted_explicit \"$pwd/outer-repo/bare-repo\"\n+'\n+\n test_done\n\nbase-commit: 2807bd2c10606e590886543afe4e4f208dddd489\n-- \n2.40.1.495.gc816e09b53d-goog\n\n"},{"id":"476283","messageId":"kl6lbkj73laf.fsf@chooglen-macbookpro.roam.corp.google.com","threadId":"59665","inReplyTo":"b548d96db7052dc2cb664922fa2003fe068843cf.1682702058.git.steadmon@google.com","subject":"Re: [PATCH v2] setup: trace bare repository setups","fromName":"Glen Choo","fromEmail":"chooglen@google.com","sentAt":"2023-04-28T18:37:44Z","receivedAt":"2023-04-28T18:37:49Z","isPatch":true,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"Josh Steadmon <steadmon@google.com> writes:\n\n> diff --git a/t/t0035-safe-bare-repository.sh b/t/t0035-safe-bare-repository.sh\n> index 11c15a48aa..993f5bdc7d 100755\n> --- a/t/t0035-safe-bare-repository.sh\n> +++ b/t/t0035-safe-bare-repository.sh\n> @@ -7,13 +7,24 @@ TEST_PASSES_SANITIZE_LEAK=true\n>  \n>  pwd=\"$(pwd)\"\n>  \n> -expect_accepted () {\n> -\tgit \"$@\" rev-parse --git-dir\n> +expect_accepted_implicit () {\n> +\ttest_when_finished 'rm \"$pwd/trace.perf\"' &&\n> +\tGIT_TRACE2_PERF=\"$pwd/trace.perf\" git \"$@\" rev-parse --git-dir &&\n> +\tgrep -F \"implicit-bare-repository:$pwd\" \"$pwd/trace.perf\"\n> +}\n> +\n> +expect_accepted_explicit () {\n> +\ttest_when_finished 'rm \"$pwd/trace.perf\"' &&\n> +\tGIT_DIR=\"$1\" GIT_TRACE2_PERF=\"$pwd/trace.perf\" git rev-parse --git-dir &&\n> +\t! grep -F \"implicit-bare-repository:$pwd\" \"$pwd/trace.perf\"\n>  }\n\n(Not new in v2) This wasn't obvious to me at first, but\n\n  grep -F \"implicit-bare-repository:$pwd\" \"$pwd/trace.perf\"\n\nlooks like it's asserting that we trace that the bare repository is\n$pwd, but it's actually only asserting that is starts with $pwd. If it\nmight trip up other reasers, a comment here would be helpful.\n\n> @@ -21,13 +32,9 @@ test_expect_success 'setup bare repo in worktree' '\n>  \tgit init --bare outer-repo/bare-repo\n>  '\n>  \n> -test_expect_success 'safe.bareRepository unset' '\n> -\texpect_accepted -C outer-repo/bare-repo\n> -'\n> -\n\nI found this surprising; I thought it was quite valuable to test the\ndefault behavior. I'd prefer to keep this test.\n\nPerhaps there was a miscommunication? You mentioned:\n\n  Actually, explicitly setting the variable here is equivalent to the\n  following test case, so I'll just remove this one.\n\nwhich is true, but I think Junio meant _un_setting the variable,\nsomething like:\n\n  test_expect_success 'safe.bareRepository unset' '\n+   test_unconfig --global safe.bareRepository &&\n    expect_accepted -C outer-repo/bare-repo\n  '\n"},{"id":"476292","messageId":"xmqq354jda8g.fsf@gitster.g","threadId":"59665","inReplyTo":"ZEv8AcxQW7B2H8uJ@google.com","subject":"Re: [PATCH] setup: trace bare repository setups","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-04-28T20:26:23Z","receivedAt":"2023-04-28T20:26:29Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Josh Steadmon <steadmon@google.com> writes:\n\n> On 2023.04.27 15:54, Junio C Hamano wrote:\n>> Josh Steadmon <steadmon@google.com> writes:\n>> \n>> > @@ -22,12 +33,12 @@ test_expect_success 'setup bare repo in worktree' '\n>> >  '\n>> >  \n>> >  test_expect_success 'safe.bareRepository unset' '\n>> > -\texpect_accepted -C outer-repo/bare-repo\n>> > +\texpect_accepted_implicit -C outer-repo/bare-repo\n>> >  '\n>> \n>> Perhaps futureproof this test piece by explicitly unsetting the\n>> variable before starting the test?  That way, this piece will not be\n>> broken even if earlier tests gets modified to set some value to\n>> safe.bareRepository in the future.\n>\n> Actually, explicitly setting the variable here is equivalent to the\n> following test case, so I'll just remove this one.\n\nI meant explicitly UNsetting, though?\n"},{"id":"476361","messageId":"ZE/03oSVAmSFLh7c@google.com","threadId":"59665","inReplyTo":"xmqq354jda8g.fsf@gitster.g","subject":"Re: [PATCH] setup: trace bare repository setups","fromName":"Josh Steadmon","fromEmail":"steadmon@google.com","sentAt":"2023-05-01T17:20:53Z","receivedAt":"2023-05-01T17:21:03Z","isPatch":true,"sender":{"key":"steadmon@google.com","avatar":"https://avatars.githubusercontent.com/u/2654920?v=4"},"body":"On 2023.04.28 13:26, Junio C Hamano wrote:\n> Josh Steadmon <steadmon@google.com> writes:\n> \n> > On 2023.04.27 15:54, Junio C Hamano wrote:\n> >> Josh Steadmon <steadmon@google.com> writes:\n> >> \n> >> > @@ -22,12 +33,12 @@ test_expect_success 'setup bare repo in worktree' '\n> >> >  '\n> >> >  \n> >> >  test_expect_success 'safe.bareRepository unset' '\n> >> > -\texpect_accepted -C outer-repo/bare-repo\n> >> > +\texpect_accepted_implicit -C outer-repo/bare-repo\n> >> >  '\n> >> \n> >> Perhaps futureproof this test piece by explicitly unsetting the\n> >> variable before starting the test?  That way, this piece will not be\n> >> broken even if earlier tests gets modified to set some value to\n> >> safe.bareRepository in the future.\n> >\n> > Actually, explicitly setting the variable here is equivalent to the\n> > following test case, so I'll just remove this one.\n> \n> I meant explicitly UNsetting, though?\n\nAh, sorry for misunderstanding. I've restored the test along with a\ntest_unconfig line for V3; however, doesn't this just turn into a \"change\ndetector\" test whose only purpose will be to fail if/when we change the\ndefault value for this config option? \n"},{"id":"476362","messageId":"ZE/1bRh+pisYWBrV@google.com","threadId":"59665","inReplyTo":"kl6lbkj73laf.fsf@chooglen-macbookpro.roam.corp.google.com","subject":"Re: [PATCH v2] setup: trace bare repository setups","fromName":"Josh Steadmon","fromEmail":"steadmon@google.com","sentAt":"2023-05-01T17:22:53Z","receivedAt":"2023-05-01T17:23:04Z","isPatch":true,"sender":{"key":"steadmon@google.com","avatar":"https://avatars.githubusercontent.com/u/2654920?v=4"},"body":"On 2023.04.28 11:37, Glen Choo wrote:\n> Josh Steadmon <steadmon@google.com> writes:\n> \n> > diff --git a/t/t0035-safe-bare-repository.sh b/t/t0035-safe-bare-repository.sh\n> > index 11c15a48aa..993f5bdc7d 100755\n> > --- a/t/t0035-safe-bare-repository.sh\n> > +++ b/t/t0035-safe-bare-repository.sh\n> > @@ -7,13 +7,24 @@ TEST_PASSES_SANITIZE_LEAK=true\n> >  \n> >  pwd=\"$(pwd)\"\n> >  \n> > -expect_accepted () {\n> > -\tgit \"$@\" rev-parse --git-dir\n> > +expect_accepted_implicit () {\n> > +\ttest_when_finished 'rm \"$pwd/trace.perf\"' &&\n> > +\tGIT_TRACE2_PERF=\"$pwd/trace.perf\" git \"$@\" rev-parse --git-dir &&\n> > +\tgrep -F \"implicit-bare-repository:$pwd\" \"$pwd/trace.perf\"\n> > +}\n> > +\n> > +expect_accepted_explicit () {\n> > +\ttest_when_finished 'rm \"$pwd/trace.perf\"' &&\n> > +\tGIT_DIR=\"$1\" GIT_TRACE2_PERF=\"$pwd/trace.perf\" git rev-parse --git-dir &&\n> > +\t! grep -F \"implicit-bare-repository:$pwd\" \"$pwd/trace.perf\"\n> >  }\n> \n> (Not new in v2) This wasn't obvious to me at first, but\n> \n>   grep -F \"implicit-bare-repository:$pwd\" \"$pwd/trace.perf\"\n> \n> looks like it's asserting that we trace that the bare repository is\n> $pwd, but it's actually only asserting that is starts with $pwd. If it\n> might trip up other reasers, a comment here would be helpful.\n\nAdded a note in V3.\n\n\n> > @@ -21,13 +32,9 @@ test_expect_success 'setup bare repo in worktree' '\n> >  \tgit init --bare outer-repo/bare-repo\n> >  '\n> >  \n> > -test_expect_success 'safe.bareRepository unset' '\n> > -\texpect_accepted -C outer-repo/bare-repo\n> > -'\n> > -\n> \n> I found this surprising; I thought it was quite valuable to test the\n> default behavior. I'd prefer to keep this test.\n> \n> Perhaps there was a miscommunication? You mentioned:\n> \n>   Actually, explicitly setting the variable here is equivalent to the\n>   following test case, so I'll just remove this one.\n> \n> which is true, but I think Junio meant _un_setting the variable,\n> something like:\n> \n>   test_expect_success 'safe.bareRepository unset' '\n> +   test_unconfig --global safe.bareRepository &&\n>     expect_accepted -C outer-repo/bare-repo\n>   '\n\nYeah, misunderstood. However, see my reply to Junio about this being a\n\"change detector\" test.\n"},{"id":"476364","messageId":"e98be8e7f703fc741e06d9208545abc8c24d1a4a.1682962110.git.steadmon@google.com","threadId":"59665","inReplyTo":"cb72bca46c6ff2a8cf3196408fb53411f7f91892.1682631601.git.steadmon@google.com","subject":"[PATCH v3] setup: trace bare repository setups","fromName":"Josh Steadmon","fromEmail":"steadmon@google.com","sentAt":"2023-05-01T17:30:37Z","receivedAt":"2023-05-01T17:30:43Z","isPatch":true,"sender":{"key":"steadmon@google.com","avatar":"https://avatars.githubusercontent.com/u/2654920?v=4"},"body":"From: Glen Choo <chooglen@google.com>\n\nsafe.bareRepository=explicit is a safer default mode of operation, since\nit guards against the embedded bare repository attack [1]. Most end\nusers don't use bare repositories directly, so they should be able to\nset safe.bareRepository=explicit, with the expectation that they can\nreenable bare repositories by specifying GIT_DIR or --git-dir.\n\nHowever, the user might use a tool that invokes Git on bare repositories\nwithout setting GIT_DIR (e.g. \"go mod\" will clone bare repositories\n[2]), so even if a user wanted to use safe.bareRepository=explicit, it\nwouldn't be feasible until their tools learned to set GIT_DIR.\n\nTo make this transition easier, add a trace message to note when we\nattempt to set up a bare repository without setting GIT_DIR. This allows\nusers and tool developers to audit which of their tools are problematic\nand report/fix the issue.  When they are sufficiently confident, they\nwould switch over to \"safe.bareRepository=explicit\".\n\nNote that this uses trace2_data_string(), which isn't supported by the\n\"normal\" GIT_TRACE2 target, only _EVENT or _PERF.\n\n[1] https://lore.kernel.org/git/kl6lsfqpygsj.fsf@chooglen-macbookpro.roam.corp.google.com/\n[2] https://go.dev/ref/mod\n\nSigned-off-by: Glen Choo <chooglen@google.com>\nSigned-off-by: Josh Steadmon <steadmon@google.com>\n---\nI'm sending a lightly-adapted version of Glen's tracing patch because\nGlen will be on vacation next week and we'd like to get this upstream\nASAP.\n\nChanges in V3: added a test_unconfig test case for safe.bareRepository\nChanges in V2: cleaned up test-style issues.\n\nRange-diff against v2:\n1:  b548d96db7 ! 1:  e98be8e7f7 setup: trace bare repository setups\n    @@ t/t0035-safe-bare-repository.sh: TEST_PASSES_SANITIZE_LEAK=true\n     +expect_accepted_implicit () {\n     +\ttest_when_finished 'rm \"$pwd/trace.perf\"' &&\n     +\tGIT_TRACE2_PERF=\"$pwd/trace.perf\" git \"$@\" rev-parse --git-dir &&\n    ++\t# Note: we're intentionally only checking that the bare repo has a\n    ++\t# directory *prefix* of $pwd\n     +\tgrep -F \"implicit-bare-repository:$pwd\" \"$pwd/trace.perf\"\n     +}\n     +\n    @@ t/t0035-safe-bare-repository.sh: TEST_PASSES_SANITIZE_LEAK=true\n      \n      test_expect_success 'setup bare repo in worktree' '\n     @@ t/t0035-safe-bare-repository.sh: test_expect_success 'setup bare repo in worktree' '\n    - \tgit init --bare outer-repo/bare-repo\n      '\n      \n    --test_expect_success 'safe.bareRepository unset' '\n    + test_expect_success 'safe.bareRepository unset' '\n     -\texpect_accepted -C outer-repo/bare-repo\n    --'\n    --\n    ++\ttest_unconfig --global safe.bareRepository &&\n    ++\texpect_accepted_implicit -C outer-repo/bare-repo\n    + '\n    + \n      test_expect_success 'safe.bareRepository=all' '\n      \ttest_config_global safe.bareRepository all &&\n     -\texpect_accepted -C outer-repo/bare-repo\n\n setup.c                         |  1 +\n t/t0035-safe-bare-repository.sh | 32 +++++++++++++++++++++++++-------\n 2 files changed, 26 insertions(+), 7 deletions(-)\n\ndiff --git a/setup.c b/setup.c\nindex 59abc16ba6..458582207e 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -1352,6 +1352,7 @@ static enum discovery_result setup_git_directory_gently_1(struct strbuf *dir,\n \t\t}\n \n \t\tif (is_git_directory(dir->buf)) {\n+\t\t\ttrace2_data_string(\"setup\", NULL, \"implicit-bare-repository\", dir->buf);\n \t\t\tif (get_allowed_bare_repo() == ALLOWED_BARE_REPO_EXPLICIT)\n \t\t\t\treturn GIT_DIR_DISALLOWED_BARE;\n \t\t\tif (!ensure_valid_ownership(NULL, NULL, dir->buf, report))\ndiff --git a/t/t0035-safe-bare-repository.sh b/t/t0035-safe-bare-repository.sh\nindex 11c15a48aa..038b8b788d 100755\n--- a/t/t0035-safe-bare-repository.sh\n+++ b/t/t0035-safe-bare-repository.sh\n@@ -7,13 +7,26 @@ TEST_PASSES_SANITIZE_LEAK=true\n \n pwd=\"$(pwd)\"\n \n-expect_accepted () {\n-\tgit \"$@\" rev-parse --git-dir\n+expect_accepted_implicit () {\n+\ttest_when_finished 'rm \"$pwd/trace.perf\"' &&\n+\tGIT_TRACE2_PERF=\"$pwd/trace.perf\" git \"$@\" rev-parse --git-dir &&\n+\t# Note: we're intentionally only checking that the bare repo has a\n+\t# directory *prefix* of $pwd\n+\tgrep -F \"implicit-bare-repository:$pwd\" \"$pwd/trace.perf\"\n+}\n+\n+expect_accepted_explicit () {\n+\ttest_when_finished 'rm \"$pwd/trace.perf\"' &&\n+\tGIT_DIR=\"$1\" GIT_TRACE2_PERF=\"$pwd/trace.perf\" git rev-parse --git-dir &&\n+\t! grep -F \"implicit-bare-repository:$pwd\" \"$pwd/trace.perf\"\n }\n \n expect_rejected () {\n-\ttest_must_fail git \"$@\" rev-parse --git-dir 2>err &&\n-\tgrep -F \"cannot use bare repository\" err\n+\ttest_when_finished 'rm \"$pwd/trace.perf\"' &&\n+\ttest_env GIT_TRACE2_PERF=\"$pwd/trace.perf\" \\\n+\t\ttest_must_fail git \"$@\" rev-parse --git-dir 2>err &&\n+\tgrep -F \"cannot use bare repository\" err &&\n+\tgrep -F \"implicit-bare-repository:$pwd\" \"$pwd/trace.perf\"\n }\n \n test_expect_success 'setup bare repo in worktree' '\n@@ -22,12 +35,13 @@ test_expect_success 'setup bare repo in worktree' '\n '\n \n test_expect_success 'safe.bareRepository unset' '\n-\texpect_accepted -C outer-repo/bare-repo\n+\ttest_unconfig --global safe.bareRepository &&\n+\texpect_accepted_implicit -C outer-repo/bare-repo\n '\n \n test_expect_success 'safe.bareRepository=all' '\n \ttest_config_global safe.bareRepository all &&\n-\texpect_accepted -C outer-repo/bare-repo\n+\texpect_accepted_implicit -C outer-repo/bare-repo\n '\n \n test_expect_success 'safe.bareRepository=explicit' '\n@@ -47,7 +61,7 @@ test_expect_success 'safe.bareRepository in the repository' '\n \n test_expect_success 'safe.bareRepository on the command line' '\n \ttest_config_global safe.bareRepository explicit &&\n-\texpect_accepted -C outer-repo/bare-repo \\\n+\texpect_accepted_implicit -C outer-repo/bare-repo \\\n \t\t-c safe.bareRepository=all\n '\n \n@@ -60,4 +74,8 @@ test_expect_success 'safe.bareRepository in included file' '\n \texpect_rejected -C outer-repo/bare-repo\n '\n \n+test_expect_success 'no trace when GIT_DIR is explicitly provided' '\n+\texpect_accepted_explicit \"$pwd/outer-repo/bare-repo\"\n+'\n+\n test_done\n\nbase-commit: 2807bd2c10606e590886543afe4e4f208dddd489\n-- \n2.40.1.495.gc816e09b53d-goog\n\n"},{"id":"476658","messageId":"xmqqa5yicsxd.fsf@gitster.g","threadId":"59665","inReplyTo":"e98be8e7f703fc741e06d9208545abc8c24d1a4a.1682962110.git.steadmon@google.com","subject":"Re: [PATCH v3] setup: trace bare repository setups","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-05-05T22:30:38Z","receivedAt":"2023-05-05T22:30:47Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Josh Steadmon <steadmon@google.com> writes:\n\n> From: Glen Choo <chooglen@google.com>\n>\n> safe.bareRepository=explicit is a safer default mode of operation, since\n> it guards against the embedded bare repository attack [1]. Most end\n> users don't use bare repositories directly, so they should be able to\n> set safe.bareRepository=explicit, with the expectation that they can\n> reenable bare repositories by specifying GIT_DIR or --git-dir.\n>\n> However, the user might use a tool that invokes Git on bare repositories\n> without setting GIT_DIR (e.g. \"go mod\" will clone bare repositories\n> [2]), so even if a user wanted to use safe.bareRepository=explicit, it\n> wouldn't be feasible until their tools learned to set GIT_DIR.\n>\n> To make this transition easier, add a trace message to note when we\n> attempt to set up a bare repository without setting GIT_DIR. This allows\n> users and tool developers to audit which of their tools are problematic\n> and report/fix the issue.  When they are sufficiently confident, they\n> would switch over to \"safe.bareRepository=explicit\".\n>\n> Note that this uses trace2_data_string(), which isn't supported by the\n> \"normal\" GIT_TRACE2 target, only _EVENT or _PERF.\n>\n> [1] https://lore.kernel.org/git/kl6lsfqpygsj.fsf@chooglen-macbookpro.roam.corp.google.com/\n> [2] https://go.dev/ref/mod\n>\n> Signed-off-by: Glen Choo <chooglen@google.com>\n> Signed-off-by: Josh Steadmon <steadmon@google.com>\n> ---\n> I'm sending a lightly-adapted version of Glen's tracing patch because\n> Glen will be on vacation next week and we'd like to get this upstream\n> ASAP.\n>\n> Changes in V3: added a test_unconfig test case for safe.bareRepository\n> Changes in V2: cleaned up test-style issues.\n\nThanks.  We saw no interest on the list in reviewing this patch\nfurther, it seems, but I didn't see anything glaringly wrong, see\nno reason not to merge it, and this should help noticing potential\nissues by $corp folks, I would presume, so let's merge it as-is.\n"},{"id":"476835","messageId":"kl6lo7mu1n5v.fsf@chooglen-macbookpro.roam.corp.google.com","threadId":"59665","inReplyTo":"ZE/03oSVAmSFLh7c@google.com","subject":"Re: [PATCH] setup: trace bare repository setups","fromName":"Glen Choo","fromEmail":"chooglen@google.com","sentAt":"2023-05-08T22:19:40Z","receivedAt":"2023-05-08T22:19:52Z","isPatch":true,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"Josh Steadmon <steadmon@google.com> writes:\n\n> Ah, sorry for misunderstanding. I've restored the test along with a\n> test_unconfig line for V3; however, doesn't this just turn into a \"change\n> detector\" test whose only purpose will be to fail if/when we change the\n> default value for this config option? \n\nMaybe a difference in opinion: \"change detection\" sounds like\nnon-essential changes to program behavior can cause a test to fail, but\ndefault value handling seems quite essential to me. Not to mention that\ndefault value handling is easy to break, so IMO guarding against\naccidental regressions is pretty useful.\n\nAs for practical churn, I don't expect that we'll intentionally change\nthis default often.\n"},{"id":"476840","messageId":"ZFl4UVhoR4Hlm2g3@nand.local","threadId":"59665","inReplyTo":"xmqqa5yicsxd.fsf@gitster.g","subject":"Re: [PATCH v3] setup: trace bare repository setups","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-05-08T22:31:45Z","receivedAt":"2023-05-08T22:31:51Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Fri, May 05, 2023 at 03:30:38PM -0700, Junio C Hamano wrote:\n> Thanks.  We saw no interest on the list in reviewing this patch\n> further, it seems, but I didn't see anything glaringly wrong, see\n> no reason not to merge it, and this should help noticing potential\n> issues by $corp folks, I would presume, so let's merge it as-is.\n\nI took a look through this thread and would be fine to see this one\npicked up, though I did have a couple of questions:\n\n  - Is the plan to eventually disable $GIT_DIR discovery in bare\n    repositories by default in a future version? I am still uncertain\n    of the assumption that most end-users don't interact with bare\n    repositories directly.\n\n    Certainly forges touch bare repositories without always setting\n    $GIT_DIR in their environment. But I would imagine that other tools\n    indirectly touch bare repositories on behalf of the user. You\n    mentioned \"go\" as one such tool that doesn't set $GIT_DIR, I imagine\n    there are many more.\n\n  - If it is the plan to disable $GIT_DIR discovery in bare repositories\n    in the future, I'm not sure how visible the extra trace line would\n    be. Perhaps that is desirable, since having an advise() call on\n    every Git invocation in a bare repository would be noisy.\n\nThanks,\nTaylor\n"},{"id":"477011","messageId":"ZFwop6NTThvLKOU3@google.com","threadId":"59665","inReplyTo":"ZFl4UVhoR4Hlm2g3@nand.local","subject":"Re: [PATCH v3] setup: trace bare repository setups","fromName":"Josh Steadmon","fromEmail":"steadmon@google.com","sentAt":"2023-05-10T23:29:05Z","receivedAt":"2023-05-10T23:29:17Z","isPatch":true,"sender":{"key":"steadmon@google.com","avatar":"https://avatars.githubusercontent.com/u/2654920?v=4"},"body":"On 2023.05.08 18:31, Taylor Blau wrote:\n> On Fri, May 05, 2023 at 03:30:38PM -0700, Junio C Hamano wrote:\n> > Thanks.  We saw no interest on the list in reviewing this patch\n> > further, it seems, but I didn't see anything glaringly wrong, see\n> > no reason not to merge it, and this should help noticing potential\n> > issues by $corp folks, I would presume, so let's merge it as-is.\n> \n> I took a look through this thread and would be fine to see this one\n> picked up, though I did have a couple of questions:\n> \n>   - Is the plan to eventually disable $GIT_DIR discovery in bare\n>     repositories by default in a future version? I am still uncertain\n>     of the assumption that most end-users don't interact with bare\n>     repositories directly.\n\nI think at some point in the future, we'd like for\n`safe.bareRepository=explicit` to be the default. To get to a state\nwhere we're comfortable making that change, we plan in the near-ish\nfuture to flip the default when `feature.experimental` is enabled.\n\n>     Certainly forges touch bare repositories without always setting\n>     $GIT_DIR in their environment. But I would imagine that other tools\n>     indirectly touch bare repositories on behalf of the user. You\n>     mentioned \"go\" as one such tool that doesn't set $GIT_DIR, I imagine\n>     there are many more.\n> \n>   - If it is the plan to disable $GIT_DIR discovery in bare repositories\n>     in the future, I'm not sure how visible the extra trace line would\n>     be. Perhaps that is desirable, since having an advise() call on\n>     every Git invocation in a bare repository would be noisy.\n\nYeah, this is mainly to help us plan for our internal rollout of\n`safe.bareRepository=explicit` at $DAYJOB, but we assume it would be\nhelpful for others who might also be considering this, or for developers\nof affected tooling when they receive a bug report that bareRepository\nbreaks their users.\n\nWe also want to add some more detailed tracing in general around\nrepo/worktree initialization, but that part is not so urgent.\n"}]}