{"thread":{"id":"55158","subject":"[PATCH] t/perf: handle worktrees as test repos","startedAt":"2021-02-16T20:13:30Z","lastAt":"2021-03-01T22:10:16Z","messageCount":12,"participants":["Jeff King","Derrick Stolee","Johannes Schindelin","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"417101","messageId":"YCwnPVFsYDa0SNmG@coredump.intra.peff.net","threadId":"55158","inReplyTo":null,"subject":"[PATCH] t/perf: handle worktrees as test repos","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-02-16T20:12:45Z","receivedAt":"2021-02-16T20:13:30Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The perf suite gets confused when test_perf_default_repo is pointed at a\nworktree (which includes when it is run from within a worktree at all,\nsince the default is to use the current repository).\n\nHere's an example:\n\n $ git worktree add ~/foo\n Preparing worktree (new branch 'foo')\n HEAD is now at 328c109303 The eighth batch\n $ cd ~/foo\n $ make\n [...build output...]\n $ cd t/perf\n $ ./p0000-perf-lib-sanity.sh -v -i\n [...]\n perf 1 - test_perf_default_repo works:\n running:\n \tfoo=$(git rev-parse HEAD) &&\n \ttest_export foo\n\n fatal: ambiguous argument 'HEAD': unknown revision or path not in the working tree.\n Use '--' to separate paths from revisions, like this:\n 'git [...] -- [...]'\n\nThe problem is that we didn't copy all of the necessary files from the\nsource repository (in this case we got HEAD, but we have no refs!). We\ndiscover the git-dir with \"rev-parse --git-dir\", but this points to the\nworktree's partial repository in .../.git/worktrees/foo.\n\nThat partial repository has a \"commondir\" file which points to the main\nrepository, where the actual refs are stored, but we don't copy it. This\nis the correct thing to do, though! If we did copy it, then our scratch\ntest repo would be pointing back to the original main repo, and any ref\nupdates we made in the tests would impact that original repo.\n\nInstead, we need to either:\n\n 1. Make a scratch copy of the original main repo (in addition to the\n worktree repo), and point the scratch worktree repo's commondir at\n it. This preserves the original relationship, but it's doubtful any\n script really cares (if they are testing worktree performance,\n they'd probably make their own worktrees). And it's trickier to get\n right.\n\n 2. Collapse the main and worktree repos into a single scratch repo.\n This can be done by copying everything from both, preferring any\n files from the worktree repo.\n\nThis patch does the second one. With this applied, the example above\nresults in p0000 running successfully.\n\nReported-by: Derrick Stolee \nSigned-off-by: Jeff King \n---\nHaving written that, it occurs to me that an even simpler solution is to\njust always use the commondir as the source of the scratch repo. It does\nnot produce the same outcome, but the point is generally just to find a\nsuitable starting point for a repository. Grabbing the main repo instead\nof one of its worktrees is probably OK for most tests.\n\n t/perf/perf-lib.sh | 31 ++++++++++++++++++++++---------\n 1 file changed, 22 insertions(+), 9 deletions(-)\n\ndiff --git a/t/perf/perf-lib.sh b/t/perf/perf-lib.sh\nindex e385c6896f..1226be4005 100644\n--- a/t/perf/perf-lib.sh\n+++ b/t/perf/perf-lib.sh\n@@ -70,27 +70,40 @@ test_perf_do_repo_symlink_config_ () {\n \ttest_have_prereq SYMLINKS || git config core.symlinks false\n }\n \n+test_perf_copy_repo_contents () {\n+\tfor stuff in \"$1\"/*\n+\tdo\n+\t\tcase \"$stuff\" in\n+\t\t*/objects|*/hooks|*/config|*/commondir)\n+\t\t\t;;\n+\t\t*)\n+\t\t\tcp -R \"$stuff\" \"$repo/.git/\" || exit 1\n+\t\t\t;;\n+\t\tesac\n+\tdone\n+}\n+\n test_perf_create_repo_from () {\n \ttest \"$#\" = 2 ||\n \tBUG \"not 2 parameters to test-create-repo\"\n \trepo=\"$1\"\n \tsource=\"$2\"\n \tsource_git=\"$(\"$MODERN_GIT\" -C \"$source\" rev-parse --git-dir)\"\n \tobjects_dir=\"$(\"$MODERN_GIT\" -C \"$source\" rev-parse --git-path objects)\"\n+\tcommon_dir=\"$(\"$MODERN_GIT\" -C \"$source\" rev-parse --git-common-dir)\"\n \tmkdir -p \"$repo/.git\"\n \t(\n \t\tcd \"$source\" &&\n \t\t{ cp -Rl \"$objects_dir\" \"$repo/.git/\" 2>/dev/null ||\n \t\t\tcp -R \"$objects_dir\" \"$repo/.git/\"; } &&\n-\t\tfor stuff in \"$source_git\"/*; do\n-\t\t\tcase \"$stuff\" in\n-\t\t\t\t*/objects|*/hooks|*/config|*/commondir)\n-\t\t\t\t\t;;\n-\t\t\t\t*)\n-\t\t\t\t\tcp -R \"$stuff\" \"$repo/.git/\" || exit 1\n-\t\t\t\t\t;;\n-\t\t\tesac\n-\t\tdone\n+\n+\t\t# common_dir must come first here, since we want source_git to\n+\t\t# take precedence and overwrite any overlapping files\n+\t\ttest_perf_copy_repo_contents \"$common_dir\"\n+\t\tif test \"$source_git\" != \"$common_dir\"\n+\t\tthen\n+\t\t\ttest_perf_copy_repo_contents \"$source_git\"\n+\t\tfi\n \t) &&\n \t(\n \t\tcd \"$repo\" &&\n-- \n2.30.1.989.g5e01c2f281\n"},{"id":"417102","messageId":"YCwoOMo7obrNOGYX@coredump.intra.peff.net","threadId":"55158","inReplyTo":"YCwnPVFsYDa0SNmG@coredump.intra.peff.net","subject":"Re: [PATCH] t/perf: handle worktrees as test repos","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-02-16T20:16:56Z","receivedAt":"2021-02-16T20:17:53Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Feb 16, 2021 at 03:12:45PM -0500, Jeff King wrote:\n\n> Having written that, it occurs to me that an even simpler solution is to\n> just always use the commondir as the source of the scratch repo. It does\n> not produce the same outcome, but the point is generally just to find a\n> suitable starting point for a repository. Grabbing the main repo instead\n> of one of its worktrees is probably OK for most tests.\n\nThe patch there is delightfully simple:\n\ndiff --git a/t/perf/perf-lib.sh b/t/perf/perf-lib.sh\nindex e385c6896f..7018256cd4 100644\n--- a/t/perf/perf-lib.sh\n+++ b/t/perf/perf-lib.sh\n@@ -75,7 +75,7 @@ test_perf_create_repo_from () {\n \tBUG \"not 2 parameters to test-create-repo\"\n \trepo=\"$1\"\n \tsource=\"$2\"\n-\tsource_git=\"$(\"$MODERN_GIT\" -C \"$source\" rev-parse --git-dir)\"\n+\tsource_git=\"$(\"$MODERN_GIT\" -C \"$source\" rev-parse --git-common-dir)\"\n \tobjects_dir=\"$(\"$MODERN_GIT\" -C \"$source\" rev-parse --git-path objects)\"\n \tmkdir -p \"$repo/.git\"\n \t(\n\nbut I do wonder if somebody would find it confusing.\n\n-Peff\n"},{"id":"417105","messageId":"22378ce3-6845-1cd9-996a-8bdc3a8b65d7@gmail.com","threadId":"55158","inReplyTo":"YCwoOMo7obrNOGYX@coredump.intra.peff.net","subject":"Re: [PATCH] t/perf: handle worktrees as test repos","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2021-02-16T20:36:04Z","receivedAt":"2021-02-16T20:37:05Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 2/16/2021 3:16 PM, Jeff King wrote:\n> On Tue, Feb 16, 2021 at 03:12:45PM -0500, Jeff King wrote:\n> \n>> Having written that, it occurs to me that an even simpler solution is to\n>> just always use the commondir as the source of the scratch repo. It does\n>> not produce the same outcome, but the point is generally just to find a\n>> suitable starting point for a repository. Grabbing the main repo instead\n>> of one of its worktrees is probably OK for most tests.\n> \n> The patch there is delightfully simple:\n\nI do like this simplicity.\n \n> diff --git a/t/perf/perf-lib.sh b/t/perf/perf-lib.sh\n> index e385c6896f..7018256cd4 100644\n> --- a/t/perf/perf-lib.sh\n> +++ b/t/perf/perf-lib.sh\n> @@ -75,7 +75,7 @@ test_perf_create_repo_from () {\n> \tBUG \"not 2 parameters to test-create-repo\"\n> \trepo=\"$1\"\n> \tsource=\"$2\"\n> -\tsource_git=\"$(\"$MODERN_GIT\" -C \"$source\" rev-parse --git-dir)\"\n> +\tsource_git=\"$(\"$MODERN_GIT\" -C \"$source\" rev-parse --git-common-dir)\"\n> \tobjects_dir=\"$(\"$MODERN_GIT\" -C \"$source\" rev-parse --git-path objects)\"\n> \tmkdir -p \"$repo/.git\"\n> \t(\n> \n> but I do wonder if somebody would find it confusing.\n\nIt would be confusing, especially if one let the \"main\" worktree\nlanguish far behind another worktree. Rather, one case that applies\nmostly to me and my team is when we work on git-for-windows/git or\nmicrosoft/git in a worktree off of git/git. I think it would be\nappropriate to use either, as the differences at HEAD are not so\nsignificant to matter. But, any deviation from the HEAD of the\ncurrent worktree might be confusing when trying to reproduce some\nsurprising behavior.\n\nThanks,\n-Stolee\n"},{"id":"417107","messageId":"nycvar.QRO.7.76.6.2102162211050.52@tvgsbejvaqbjf.bet","threadId":"55158","inReplyTo":"YCwnPVFsYDa0SNmG@coredump.intra.peff.net","subject":"Re: [PATCH] t/perf: handle worktrees as test repos","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2021-02-16T21:13:49Z","receivedAt":"2021-02-16T21:15:47Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Peff,\n\nOn Tue, 16 Feb 2021, Jeff King wrote:\n\n> The perf suite gets confused when test_perf_default_repo is pointed at a\n> worktree (which includes when it is run from within a worktree at all,\n> since the default is to use the current repository).\n>\n> Here's an example:\n>\n> $ git worktree add ~/foo\n> Preparing worktree (new branch 'foo')\n> HEAD is now at 328c109303 The eighth batch\n> $ cd ~/foo\n> $ make\n> [...build output...]\n> $ cd t/perf\n> $ ./p0000-perf-lib-sanity.sh -v -i\n> [...]\n> perf 1 - test_perf_default_repo works:\n> running:\n> \tfoo=$(git rev-parse HEAD) &&\n> \ttest_export foo\n>\n> fatal: ambiguous argument 'HEAD': unknown revision or path not in the working tree.\n> Use '--' to separate paths from revisions, like this:\n> 'git [...] -- [...]'\n>\n> The problem is that we didn't copy all of the necessary files from the\n> source repository (in this case we got HEAD, but we have no refs!). We\n> discover the git-dir with \"rev-parse --git-dir\", but this points to the\n> worktree's partial repository in .../.git/worktrees/foo.\n>\n> That partial repository has a \"commondir\" file which points to the main\n> repository, where the actual refs are stored, but we don't copy it. This\n> is the correct thing to do, though! If we did copy it, then our scratch\n> test repo would be pointing back to the original main repo, and any ref\n> updates we made in the tests would impact that original repo.\n>\n> Instead, we need to either:\n>\n> 1. Make a scratch copy of the original main repo (in addition to the\n> worktree repo), and point the scratch worktree repo's commondir at\n> it. This preserves the original relationship, but it's doubtful any\n> script really cares (if they are testing worktree performance,\n> they'd probably make their own worktrees). And it's trickier to get\n> right.\n>\n> 2. Collapse the main and worktree repos into a single scratch repo.\n> This can be done by copying everything from both, preferring any\n> files from the worktree repo.\n>\n> This patch does the second one. With this applied, the example above\n> results in p0000 running successfully.\n>\n> Reported-by: Derrick Stolee \n> Signed-off-by: Jeff King \n> ---\n\nI think you'll also need the equivalent of:\n\n-- snip --\ndiff --git a/t/perf/perf-lib.sh b/t/perf/perf-lib.sh\nindex 22d727cef83..0949c360ec4 100644\n--- a/t/perf/perf-lib.sh\n+++ b/t/perf/perf-lib.sh\n@@ -84,7 +84,7 @@ test_perf_create_repo_from () {\n \t\t\tcp -R \"$objects_dir\" \"$repo/.git/\"; } &&\n \t\tfor stuff in \"$source_git\"/*; do\n \t\t\tcase \"$stuff\" in\n-\t\t\t\t*/objects|*/hooks|*/config|*/commondir)\n+\t\t\t\t*/objects|*/hooks|*/config|*/commondir|*/gitdir)\n \t\t\t\t\t;;\n \t\t\t\t*)\n \t\t\t\t\tcp -R \"$stuff\" \"$repo/.git/\" || exit 1\n-- snap --\n\n> Having written that, it occurs to me that an even simpler solution is to\n> just always use the commondir as the source of the scratch repo. It does\n> not produce the same outcome, but the point is generally just to find a\n> suitable starting point for a repository. Grabbing the main repo instead\n> of one of its worktrees is probably OK for most tests.\n\nGood point: we probably also need to exclude `*/worktrees/*`, but that is\na bit trickier as we would not want to exclude, say,\n`refs/heads/worktrees/cleanup`.\n\nCiao,\nDscho\n\n>\n> t/perf/perf-lib.sh | 31 ++++++++++++++++++++++---------\n> 1 file changed, 22 insertions(+), 9 deletions(-)\n>\n> diff --git a/t/perf/perf-lib.sh b/t/perf/perf-lib.sh\n> index e385c6896f..1226be4005 100644\n> --- a/t/perf/perf-lib.sh\n> +++ b/t/perf/perf-lib.sh\n> @@ -70,27 +70,40 @@ test_perf_do_repo_symlink_config_ () {\n> \ttest_have_prereq SYMLINKS || git config core.symlinks false\n> }\n>\n> +test_perf_copy_repo_contents () {\n> +\tfor stuff in \"$1\"/*\n> +\tdo\n> +\t\tcase \"$stuff\" in\n> +\t\t*/objects|*/hooks|*/config|*/commondir)\n> +\t\t\t;;\n> +\t\t*)\n> +\t\t\tcp -R \"$stuff\" \"$repo/.git/\" || exit 1\n> +\t\t\t;;\n> +\t\tesac\n> +\tdone\n> +}\n> +\n> test_perf_create_repo_from () {\n> \ttest \"$#\" = 2 ||\n> \tBUG \"not 2 parameters to test-create-repo\"\n> \trepo=\"$1\"\n> \tsource=\"$2\"\n> \tsource_git=\"$(\"$MODERN_GIT\" -C \"$source\" rev-parse --git-dir)\"\n> \tobjects_dir=\"$(\"$MODERN_GIT\" -C \"$source\" rev-parse --git-path objects)\"\n> +\tcommon_dir=\"$(\"$MODERN_GIT\" -C \"$source\" rev-parse --git-common-dir)\"\n> \tmkdir -p \"$repo/.git\"\n> \t(\n> \t\tcd \"$source\" &&\n> \t\t{ cp -Rl \"$objects_dir\" \"$repo/.git/\" 2>/dev/null ||\n> \t\t\tcp -R \"$objects_dir\" \"$repo/.git/\"; } &&\n> -\t\tfor stuff in \"$source_git\"/*; do\n> -\t\t\tcase \"$stuff\" in\n> -\t\t\t\t*/objects|*/hooks|*/config|*/commondir)\n> -\t\t\t\t\t;;\n> -\t\t\t\t*)\n> -\t\t\t\t\tcp -R \"$stuff\" \"$repo/.git/\" || exit 1\n> -\t\t\t\t\t;;\n> -\t\t\tesac\n> -\t\tdone\n> +\n> +\t\t# common_dir must come first here, since we want source_git to\n> +\t\t# take precedence and overwrite any overlapping files\n> +\t\ttest_perf_copy_repo_contents \"$common_dir\"\n> +\t\tif test \"$source_git\" != \"$common_dir\"\n> +\t\tthen\n> +\t\t\ttest_perf_copy_repo_contents \"$source_git\"\n> +\t\tfi\n> \t) &&\n> \t(\n> \t\tcd \"$repo\" &&\n> --\n> 2.30.1.989.g5e01c2f281\n>\n>\n"},{"id":"417109","messageId":"YCw7PcqLUFmLJj7K@coredump.intra.peff.net","threadId":"55158","inReplyTo":"nycvar.QRO.7.76.6.2102162211050.52@tvgsbejvaqbjf.bet","subject":"Re: [PATCH] t/perf: handle worktrees as test repos","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-02-16T21:38:05Z","receivedAt":"2021-02-16T21:39:21Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Feb 16, 2021 at 10:13:49PM +0100, Johannes Schindelin wrote:\n\n> I think you'll also need the equivalent of:\n> \n> -- snip --\n> diff --git a/t/perf/perf-lib.sh b/t/perf/perf-lib.sh\n> index 22d727cef83..0949c360ec4 100644\n> --- a/t/perf/perf-lib.sh\n> +++ b/t/perf/perf-lib.sh\n> @@ -84,7 +84,7 @@ test_perf_create_repo_from () {\n> \t\t\tcp -R \"$objects_dir\" \"$repo/.git/\"; } &&\n> \t\tfor stuff in \"$source_git\"/*; do\n> \t\t\tcase \"$stuff\" in\n> -\t\t\t\t*/objects|*/hooks|*/config|*/commondir)\n> +\t\t\t\t*/objects|*/hooks|*/config|*/commondir|*/gitdir)\n> \t\t\t\t\t;;\n> \t\t\t\t*)\n> \t\t\t\t\tcp -R \"$stuff\" \"$repo/.git/\" || exit 1\n> -- snap --\n\nI think that's reasonable to do, but isn't it orthogonal?\n\nMy patch is fixing the case that we do not copy enough files from a\nworkdir.\n\nBoth before and after my patch, we'd be copying the gitdir file. I don't\nthink it would actually cause a problem in practice, since a \"gitdir\"\nfile in the main repo dir doesn't have any meaning. But I do think it's\nprudent to avoid copying it (just as we avoid commondir) to avoid any\nconfusion, or commands accidentally touching the original repository.\n\nLikewise...\n\n> > Having written that, it occurs to me that an even simpler solution is to\n> > just always use the commondir as the source of the scratch repo. It does\n> > not produce the same outcome, but the point is generally just to find a\n> > suitable starting point for a repository. Grabbing the main repo instead\n> > of one of its worktrees is probably OK for most tests.\n> \n> Good point: we probably also need to exclude `*/worktrees/*`, but that is\n> a bit trickier as we would not want to exclude, say,\n> `refs/heads/worktrees/cleanup`.\n\nYes, for the same reason, I think we should exclude the whole worktrees\ndirectory. I don't think we have to worry about that case (and if we\ndid, we'd already have trouble with \"refs/heads/config\" or similar). The\nreason is that the case statement is only looking at the glob made from\nthe top-level. The actual recursive expansion of \"refs/\", etc, is done\nby \"cp -R\".\n\nAnyway, what I'm suggesting is that it would be a separate patch to\navoid looking at gitdir and worktrees, in order to increase overall\nsafety. Do you want to do that on top, or should I?\n\n-Peff\n"},{"id":"417117","messageId":"xmqqtuqbocae.fsf@gitster.c.googlers.com","threadId":"55158","inReplyTo":"YCwoOMo7obrNOGYX@coredump.intra.peff.net","subject":"Re: [PATCH] t/perf: handle worktrees as test repos","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-02-16T22:52:57Z","receivedAt":"2021-02-16T22:53:47Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Tue, Feb 16, 2021 at 03:12:45PM -0500, Jeff King wrote:\n>\n>> Having written that, it occurs to me that an even simpler solution is to\n>> just always use the commondir as the source of the scratch repo. It does\n>> not produce the same outcome, but the point is generally just to find a\n>> suitable starting point for a repository. Grabbing the main repo instead\n>> of one of its worktrees is probably OK for most tests.\n>\n> The patch there is delightfully simple:\n>\n> diff --git a/t/perf/perf-lib.sh b/t/perf/perf-lib.sh\n> index e385c6896f..7018256cd4 100644\n> --- a/t/perf/perf-lib.sh\n> +++ b/t/perf/perf-lib.sh\n> @@ -75,7 +75,7 @@ test_perf_create_repo_from () {\n> \tBUG \"not 2 parameters to test-create-repo\"\n> \trepo=\"$1\"\n> \tsource=\"$2\"\n> -\tsource_git=\"$(\"$MODERN_GIT\" -C \"$source\" rev-parse --git-dir)\"\n> +\tsource_git=\"$(\"$MODERN_GIT\" -C \"$source\" rev-parse --git-common-dir)\"\n> \tobjects_dir=\"$(\"$MODERN_GIT\" -C \"$source\" rev-parse --git-path objects)\"\n> \tmkdir -p \"$repo/.git\"\n> \t(\n>\n> but I do wonder if somebody would find it confusing.\n\nThat does look quite a lot simpler.\n\nWhat are the possible downsides? Per-worktree references may not be\npointing at the same objects?\n"},{"id":"417118","messageId":"YCxNr+ViJsRlGbQJ@coredump.intra.peff.net","threadId":"55158","inReplyTo":"xmqqtuqbocae.fsf@gitster.c.googlers.com","subject":"Re: [PATCH] t/perf: handle worktrees as test repos","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-02-16T22:56:47Z","receivedAt":"2021-02-16T22:57:31Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Feb 16, 2021 at 02:52:57PM -0800, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > On Tue, Feb 16, 2021 at 03:12:45PM -0500, Jeff King wrote:\n> >\n> >> Having written that, it occurs to me that an even simpler solution is to\n> >> just always use the commondir as the source of the scratch repo. It does\n> >> not produce the same outcome, but the point is generally just to find a\n> >> suitable starting point for a repository. Grabbing the main repo instead\n> >> of one of its worktrees is probably OK for most tests.\n> >\n> > The patch there is delightfully simple:\n> >\n> > diff --git a/t/perf/perf-lib.sh b/t/perf/perf-lib.sh\n> > index e385c6896f..7018256cd4 100644\n> > --- a/t/perf/perf-lib.sh\n> > +++ b/t/perf/perf-lib.sh\n> > @@ -75,7 +75,7 @@ test_perf_create_repo_from () {\n> > \tBUG \"not 2 parameters to test-create-repo\"\n> > \trepo=\"$1\"\n> > \tsource=\"$2\"\n> > -\tsource_git=\"$(\"$MODERN_GIT\" -C \"$source\" rev-parse --git-dir)\"\n> > +\tsource_git=\"$(\"$MODERN_GIT\" -C \"$source\" rev-parse --git-common-dir)\"\n> > \tobjects_dir=\"$(\"$MODERN_GIT\" -C \"$source\" rev-parse --git-path objects)\"\n> > \tmkdir -p \"$repo/.git\"\n> > \t(\n> >\n> > but I do wonder if somebody would find it confusing.\n> \n> That does look quite a lot simpler.\n> \n> What are the possible downsides? Per-worktree references may not be\n> pointing at the same objects?\n\nThe main one IMHO is that HEAD would not be pointing where the user\nmight expect it to be.\n\n-Peff\n"},{"id":"417875","messageId":"YDiesofYW18Mzd9Q@coredump.intra.peff.net","threadId":"55158","inReplyTo":"YCwnPVFsYDa0SNmG@coredump.intra.peff.net","subject":"[PATCH v2] t/perf worktree improvements","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-02-26T07:09:38Z","receivedAt":"2021-02-26T07:10:27Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"I don't think v1 of this patch got picked up at all, so here it is\nagain. There was a question of whether we could do the much simpler\nsolution discussed in:\n\n https://lore.kernel.org/git/22378ce3-6845-1cd9-996a-8bdc3a8b65d7@gmail.com/\n\nBut I think it would be confusing. So patch 1 is unchanged here from v1.\n\nJohannes suggested we could add some extra protections to avoid\naccidentally modifying the original repo. Patch 2 does that.\n\n [1/2]: t/perf: handle worktrees as test repos\n [2/2]: t/perf: avoid copying worktree files from test repo\n\n t/perf/perf-lib.sh | 31 ++++++++++++++++++++++---------\n 1 file changed, 22 insertions(+), 9 deletions(-)\n\n-Peff\n"},{"id":"417876","messageId":"YDifK3oimeZ4HbJQ@coredump.intra.peff.net","threadId":"55158","inReplyTo":"YDiesofYW18Mzd9Q@coredump.intra.peff.net","subject":"[PATCH v2 1/2] t/perf: handle worktrees as test repos","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-02-26T07:11:39Z","receivedAt":"2021-02-26T07:12:39Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The perf suite gets confused when test_perf_default_repo is pointed at a\nworktree (which includes when it is run from within a worktree at all,\nsince the default is to use the current repository).\n\nHere's an example:\n\n $ git worktree add ~/foo\n Preparing worktree (new branch 'foo')\n HEAD is now at 328c109303 The eighth batch\n $ cd ~/foo\n $ make\n [...build output...]\n $ cd t/perf\n $ ./p0000-perf-lib-sanity.sh -v -i\n [...]\n perf 1 - test_perf_default_repo works:\n running:\n \tfoo=$(git rev-parse HEAD) &&\n \ttest_export foo\n\n fatal: ambiguous argument 'HEAD': unknown revision or path not in the working tree.\n Use '--' to separate paths from revisions, like this:\n 'git [...] -- [...]'\n\nThe problem is that we didn't copy all of the necessary files from the\nsource repository (in this case we got HEAD, but we have no refs!). We\ndiscover the git-dir with \"rev-parse --git-dir\", but this points to the\nworktree's partial repository in .../.git/worktrees/foo.\n\nThat partial repository has a \"commondir\" file which points to the main\nrepository, where the actual refs are stored, but we don't copy it. This\nis the correct thing to do, though! If we did copy it, then our scratch\ntest repo would be pointing back to the original main repo, and any ref\nupdates we made in the tests would impact that original repo.\n\nInstead, we need to either:\n\n 1. Make a scratch copy of the original main repo (in addition to the\n worktree repo), and point the scratch worktree repo's commondir at\n it. This preserves the original relationship, but it's doubtful any\n script really cares (if they are testing worktree performance,\n they'd probably make their own worktrees). And it's trickier to get\n right.\n\n 2. Collapse the main and worktree repos into a single scratch repo.\n This can be done by copying everything from both, preferring any\n files from the worktree repo.\n\nThis patch does the second one. With this applied, the example above\nresults in p0000 running successfully.\n\nReported-by: Derrick Stolee \nSigned-off-by: Jeff King \n---\n t/perf/perf-lib.sh | 31 ++++++++++++++++++++++---------\n 1 file changed, 22 insertions(+), 9 deletions(-)\n\ndiff --git a/t/perf/perf-lib.sh b/t/perf/perf-lib.sh\nindex e385c6896f..1226be4005 100644\n--- a/t/perf/perf-lib.sh\n+++ b/t/perf/perf-lib.sh\n@@ -70,27 +70,40 @@ test_perf_do_repo_symlink_config_ () {\n \ttest_have_prereq SYMLINKS || git config core.symlinks false\n }\n \n+test_perf_copy_repo_contents () {\n+\tfor stuff in \"$1\"/*\n+\tdo\n+\t\tcase \"$stuff\" in\n+\t\t*/objects|*/hooks|*/config|*/commondir)\n+\t\t\t;;\n+\t\t*)\n+\t\t\tcp -R \"$stuff\" \"$repo/.git/\" || exit 1\n+\t\t\t;;\n+\t\tesac\n+\tdone\n+}\n+\n test_perf_create_repo_from () {\n \ttest \"$#\" = 2 ||\n \tBUG \"not 2 parameters to test-create-repo\"\n \trepo=\"$1\"\n \tsource=\"$2\"\n \tsource_git=\"$(\"$MODERN_GIT\" -C \"$source\" rev-parse --git-dir)\"\n \tobjects_dir=\"$(\"$MODERN_GIT\" -C \"$source\" rev-parse --git-path objects)\"\n+\tcommon_dir=\"$(\"$MODERN_GIT\" -C \"$source\" rev-parse --git-common-dir)\"\n \tmkdir -p \"$repo/.git\"\n \t(\n \t\tcd \"$source\" &&\n \t\t{ cp -Rl \"$objects_dir\" \"$repo/.git/\" 2>/dev/null ||\n \t\t\tcp -R \"$objects_dir\" \"$repo/.git/\"; } &&\n-\t\tfor stuff in \"$source_git\"/*; do\n-\t\t\tcase \"$stuff\" in\n-\t\t\t\t*/objects|*/hooks|*/config|*/commondir)\n-\t\t\t\t\t;;\n-\t\t\t\t*)\n-\t\t\t\t\tcp -R \"$stuff\" \"$repo/.git/\" || exit 1\n-\t\t\t\t\t;;\n-\t\t\tesac\n-\t\tdone\n+\n+\t\t# common_dir must come first here, since we want source_git to\n+\t\t# take precedence and overwrite any overlapping files\n+\t\ttest_perf_copy_repo_contents \"$common_dir\"\n+\t\tif test \"$source_git\" != \"$common_dir\"\n+\t\tthen\n+\t\t\ttest_perf_copy_repo_contents \"$source_git\"\n+\t\tfi\n \t) &&\n \t(\n \t\tcd \"$repo\" &&\n-- \n2.31.0.rc0.520.ge7c5201139\n\n"},{"id":"417877","messageId":"YDifi8YeJCcx0FaV@coredump.intra.peff.net","threadId":"55158","inReplyTo":"YDiesofYW18Mzd9Q@coredump.intra.peff.net","subject":"[PATCH v2 2/2] t/perf: avoid copying worktree files from test repo","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-02-26T07:13:15Z","receivedAt":"2021-02-26T07:14:30Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When running the perf suite, we copy files from an existing $GIT_DIR to\na scratch repository to give us a realistic setup on which to operate.\nSince the perf scripts themselves may modify the scratch repository, we\nwant to make sure we've scrubbed any references back to the original.\n\nOne existing example is that we avoid copying the file \"commondir\" at\nthe top-level of the repository. In a worktree git-dir (e.g.,\n.git/worktrees/foo), that file contains the path to the parent\nrepository; copying it could mean ref updates in the scratch repository\naffect the original.\n\nBut there are other files we should cover, too:\n\n - \"gitdir\" in a worktree git-dir contains the path to the actual .git\n file in the working tree. We _shouldn't_ end up looking at it at\n all, since the lack of a \"commondir\" file means Git won't consider\n this to be a worktree git-dir. But it's best to err on the safe\n side.\n\n - in a parent repository that contains worktrees, the\n \"$GIT_DIR/worktrees\" directory will contain the git dirs for the\n individual worktrees. Which will themselves contain commondir and\n gitdir files that may reference the original repository. We should\n likewise remove them.\n\n Note that this does mean that the perf suite's scratch repositories\n will never have any worktrees. That's OK; we don't have any perf tests\n that are influenced by their presence. If we add any, they'd\n probably want to create the worktrees themselves anyway.\n\nThis patch adds both paths to the set of omissions in\ntest_perf_copy_repo_contents(). Note that we won't get confused here by\nmatching arbitrary names like refs/heads/commondir. This list is always\nmatching top-level entries in $GIT_DIR (we rely on \"cp -R\" to do the\nactual recursion).\n\nSuggested-by: Johannes Schindelin \nSigned-off-by: Jeff King \n---\n t/perf/perf-lib.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/t/perf/perf-lib.sh b/t/perf/perf-lib.sh\nindex 1226be4005..601d9f67dd 100644\n--- a/t/perf/perf-lib.sh\n+++ b/t/perf/perf-lib.sh\n@@ -74,7 +74,7 @@ test_perf_copy_repo_contents () {\n \tfor stuff in \"$1\"/*\n \tdo\n \t\tcase \"$stuff\" in\n-\t\t*/objects|*/hooks|*/config|*/commondir)\n+\t\t*/objects|*/hooks|*/config|*/commondir|*/gitdir|*/worktrees)\n \t\t\t;;\n \t\t*)\n \t\t\tcp -R \"$stuff\" \"$repo/.git/\" || exit 1\n-- \n2.31.0.rc0.520.ge7c5201139\n"},{"id":"417895","messageId":"d6edd2e2-a1ae-bdef-7e6e-7c1984903a49@gmail.com","threadId":"55158","inReplyTo":"YDiesofYW18Mzd9Q@coredump.intra.peff.net","subject":"Re: [PATCH v2] t/perf worktree improvements","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2021-02-26T15:43:50Z","receivedAt":"2021-02-26T15:44:33Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 2/26/2021 2:09 AM, Jeff King wrote:\n> I don't think v1 of this patch got picked up at all, so here it is\n> again. There was a question of whether we could do the much simpler\n> solution discussed in:\n> \n> https://lore.kernel.org/git/22378ce3-6845-1cd9-996a-8bdc3a8b65d7@gmail.com/\n> \n> But I think it would be confusing. So patch 1 is unchanged here from v1.\n> \n> Johannes suggested we could add some extra protections to avoid\n> accidentally modifying the original repo. Patch 2 does that.\n\nThanks. LGTM.\n\n-Stolee\n\n"},{"id":"418079","messageId":"nycvar.QRO.7.76.6.2103012302490.57@tvgsbejvaqbjf.bet","threadId":"55158","inReplyTo":"d6edd2e2-a1ae-bdef-7e6e-7c1984903a49@gmail.com","subject":"Re: [PATCH v2] t/perf worktree improvements","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2021-03-01T22:03:08Z","receivedAt":"2021-03-01T22:10:16Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Fri, 26 Feb 2021, Derrick Stolee wrote:\n\n> On 2/26/2021 2:09 AM, Jeff King wrote:\n> > I don't think v1 of this patch got picked up at all, so here it is\n> > again. There was a question of whether we could do the much simpler\n> > solution discussed in:\n> >\n> > https://lore.kernel.org/git/22378ce3-6845-1cd9-996a-8bdc3a8b65d7@gmail.com/\n> >\n> > But I think it would be confusing. So patch 1 is unchanged here from v1.\n> >\n> > Johannes suggested we could add some extra protections to avoid\n> > accidentally modifying the original repo. Patch 2 does that.\n>\n> Thanks. LGTM.\n\nTMT (To me, too)\n\nThanks,\nDscho\n"}]}