{"thread":{"id":"56556","subject":"[PATCH] t/perf/run: fix bin-wrappers computation","startedAt":"2021-09-21T15:46:17Z","lastAt":"2021-09-21T18:47:51Z","messageCount":3,"participants":["Derrick Stolee via GitGitGadget","Taylor Blau","Derrick Stolee"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"436624","messageId":"pull.1044.git.1632239172735.gitgitgadget@gmail.com","threadId":"56556","inReplyTo":null,"subject":"[PATCH] t/perf/run: fix bin-wrappers computation","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-09-21T15:46:12Z","receivedAt":"2021-09-21T15:46:17Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee <dstolee@microsoft.com>\n\nThe GIT_TEST_INSTALLED was moved from perf-lib.sh to run in df0f5021\n(perf-lib.sh: remove GIT_TEST_INSTALLED from perf-lib.sh, 2019-05-07)\nand that included a change to how it inspected the existence of a\nbin-wrappers directory. However, that included a typo that made the\nmatch of bin-wrappers never work. Specifically, the assignment was\n\n\tmydir_abs_wrappers=\"$mydir_abs_wrappers/bin-wrappers\"\n\nwhich uses the same variable before it is initialized. By changing it to\n\n\tmydir_abs_wrappers=\"$mydir_abs/bin-wrappers\"\n\nWe can correctly use the bin-wrappers directory.\n\nThis is critical to successfully computing performance of commands that\nexecute subcommands. The bin-wrappers ensure that the --exec-path is set\ncorrectly.\n\nReported-by: Victoria Dye <vdye@github.com>\nSigned-off-by: Derrick Stolee <dstolee@microsoft.com>\n---\n    t/perf/run: fix bin-wrappers computation\n    \n    Found this while we were testing sparse index improvements to 'git\n    stash', which uses a lot of subcommands.\n    \n    Thanks, -Stolee\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1044%2Fderrickstolee%2Fperf-run-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1044/derrickstolee/perf-run-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/1044\n\n t/perf/run | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/t/perf/run b/t/perf/run\nindex d19dec258a2..55219aa4056 100755\n--- a/t/perf/run\n+++ b/t/perf/run\n@@ -74,7 +74,7 @@ set_git_test_installed () {\n \tmydir=$1\n \n \tmydir_abs=$(cd $mydir && pwd)\n-\tmydir_abs_wrappers=\"$mydir_abs_wrappers/bin-wrappers\"\n+\tmydir_abs_wrappers=\"$mydir_abs/bin-wrappers\"\n \tif test -d \"$mydir_abs_wrappers\"\n \tthen\n \t\tGIT_TEST_INSTALLED=$mydir_abs_wrappers\n\nbase-commit: 225bc32a989d7a22fa6addafd4ce7dcd04675dbf\n-- \ngitgitgadget\n"},{"id":"436649","messageId":"YUobWlgSjZDGYSvK@nand.local","threadId":"56556","inReplyTo":"pull.1044.git.1632239172735.gitgitgadget@gmail.com","subject":"Re: [PATCH] t/perf/run: fix bin-wrappers computation","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2021-09-21T17:50:18Z","receivedAt":"2021-09-21T17:50:22Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Tue, Sep 21, 2021 at 03:46:12PM +0000, Derrick Stolee via GitGitGadget wrote:\n> This is critical to successfully computing performance of commands that\n> execute subcommands. The bin-wrappers ensure that the --exec-path is set\n> correctly.\n\nJust sanity-checking everything you said: with this bug, we'll set\nmydir_abs_wrappers to \"/bin-wrappers\", then realize that directory\ndoesn't exist, and fall back to GIT_TEST_INSTALLED=$mydir_abs. Putting\n'set -x' at the top of t/perf/run, we can see the relevant section:\n\n  + mydir=build/73cd7d9420bb7d75207e8149521db375c789a81c\n  + cd build/73cd7d9420bb7d75207e8149521db375c789a81c\n  + pwd\n  + mydir_abs=/home/ttaylorr/src/git/t/perf/build/73cd7d9420bb7d75207e8149521db375c789a81c\n  + mydir_abs_wrappers=/bin-wrappers\n  + test -d /bin-wrappers\n  + GIT_TEST_INSTALLED=/home/ttaylorr/src/git/t/perf/build/73cd7d9420bb7d75207e8149521db375c789a81c\n  + export GIT_TEST_INSTALLED\n\nOK. But the real problem is in t/test-lib.sh where we read\n\"$GIT_TEST_INSTALLED\". There we ask for the `--exec-path`, which appears\nto be wrong, at least in my setup. Printing out the $GIT_EXEC_PATH and\n$GIT_TEST_INSTALLED, I get:\n\n  /home/ttaylorr/local/git/ds.sparse-checkout/libexec/git-core,\n  /home/ttaylorr/src/git/t/perf/build/73cd7d9420bb7d75207e8149521db375c789a81c\n\nwhere the former is the branch I happen to have checked out, and the\nlatter is the revision that I asked to run performance tests on via\nt/perf/run.\n\nSo I think we'll run the right top-level Git command since the latter\npath ends up first in our $PATH, but the exec path is definitely wrong.\n\n> diff --git a/t/perf/run b/t/perf/run\n> index d19dec258a2..55219aa4056 100755\n> --- a/t/perf/run\n> +++ b/t/perf/run\n> @@ -74,7 +74,7 @@ set_git_test_installed () {\n>  \tmydir=$1\n>\n>  \tmydir_abs=$(cd $mydir && pwd)\n> -\tmydir_abs_wrappers=\"$mydir_abs_wrappers/bin-wrappers\"\n> +\tmydir_abs_wrappers=\"$mydir_abs/bin-wrappers\"\n\nThis fix looks obviously right to me (and gives me the expected output\nfrom above when I apply it locally).\n\nThanks,\nTaylor\n"},{"id":"436655","messageId":"e461bd13-9d8c-4d95-777b-c5859ee3f18c@gmail.com","threadId":"56556","inReplyTo":"YUobWlgSjZDGYSvK@nand.local","subject":"Re: [PATCH] t/perf/run: fix bin-wrappers computation","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2021-09-21T18:47:27Z","receivedAt":"2021-09-21T18:47:51Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 9/21/2021 1:50 PM, Taylor Blau wrote:\n> On Tue, Sep 21, 2021 at 03:46:12PM +0000, Derrick Stolee via GitGitGadget wrote:\n>> This is critical to successfully computing performance of commands that\n>> execute subcommands. The bin-wrappers ensure that the --exec-path is set\n>> correctly.\n> \n> Just sanity-checking everything you said: with this bug, we'll set\n> mydir_abs_wrappers to \"/bin-wrappers\", then realize that directory\n> doesn't exist, and fall back to GIT_TEST_INSTALLED=$mydir_abs. Putting\n> 'set -x' at the top of t/perf/run, we can see the relevant section:\n> \n>   + mydir=build/73cd7d9420bb7d75207e8149521db375c789a81c\n>   + cd build/73cd7d9420bb7d75207e8149521db375c789a81c\n>   + pwd\n>   + mydir_abs=/home/ttaylorr/src/git/t/perf/build/73cd7d9420bb7d75207e8149521db375c789a81c\n>   + mydir_abs_wrappers=/bin-wrappers\n>   + test -d /bin-wrappers\n>   + GIT_TEST_INSTALLED=/home/ttaylorr/src/git/t/perf/build/73cd7d9420bb7d75207e8149521db375c789a81c\n>   + export GIT_TEST_INSTALLED\n> \n> OK. But the real problem is in t/test-lib.sh where we read\n> \"$GIT_TEST_INSTALLED\". There we ask for the `--exec-path`, which appears\n> to be wrong, at least in my setup. Printing out the $GIT_EXEC_PATH and\n> $GIT_TEST_INSTALLED, I get:\n> \n>   /home/ttaylorr/local/git/ds.sparse-checkout/libexec/git-core,\n>   /home/ttaylorr/src/git/t/perf/build/73cd7d9420bb7d75207e8149521db375c789a81c\n> \n> where the former is the branch I happen to have checked out, and the\n> latter is the revision that I asked to run performance tests on via\n> t/perf/run.\n> \n> So I think we'll run the right top-level Git command since the latter\n> path ends up first in our $PATH, but the exec path is definitely wrong.\n\nThanks for digging into this more. This issue with GIT_EXEC_PATH explains\nsome strangeness I was seeing.\n\nPerhaps there is something more robust to be done around GIT_EXEC_PATH,\nbut the current fix satisfies my needs.\n \n>> diff --git a/t/perf/run b/t/perf/run\n>> index d19dec258a2..55219aa4056 100755\n>> --- a/t/perf/run\n>> +++ b/t/perf/run\n>> @@ -74,7 +74,7 @@ set_git_test_installed () {\n>>  \tmydir=$1\n>>\n>>  \tmydir_abs=$(cd $mydir && pwd)\n>> -\tmydir_abs_wrappers=\"$mydir_abs_wrappers/bin-wrappers\"\n>> +\tmydir_abs_wrappers=\"$mydir_abs/bin-wrappers\"\n> \n> This fix looks obviously right to me (and gives me the expected output\n> from above when I apply it locally).\n\nThanks,\n-Stolee\n"}]}