{"thread":{"id":"56325","subject":"[PATCH 0/2] test-lib-functions.sh: keep user's HOME, TERM and SHELL for 'test_pause' and 'debug'","startedAt":"2021-08-19T17:16:45Z","lastAt":"2021-09-07T06:24:53Z","messageCount":39,"participants":["Philippe Blain via GitGitGadget","Eric Sunshine","Taylor Blau","Junio C Hamano","Elijah Newren","Carlo Arenas","Philippe Blain","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"433134","messageId":"pull.1022.git.1629393395.gitgitgadget@gmail.com","threadId":"56325","inReplyTo":null,"subject":"[PATCH 0/2] test-lib-functions.sh: keep user's HOME, TERM and SHELL for 'test_pause' and 'debug'","fromName":"Philippe Blain via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-08-19T17:16:33Z","receivedAt":"2021-08-19T17:16:45Z","isPatch":true,"sender":{"key":"levraiphilippeblain@gmail.com","avatar":"https://avatars.githubusercontent.com/u/44212482?v=4"},"body":"This series proposes two small quality-of-life improvements (in my opinion)\nto the 'test_pause' and 'debug' test functions: using the original values of\nHOME and TERM (before they are changed by the test framework) and using\nSHELL instead of SHELL_PATH.\n\nThe later might be too big of a change, but I think it makes sense. We could\nadd a new GIT_TEST_* to conditionnaly change the behaviour, but I kept it\nsimple for v1.\n\nCheers, Philippe.\n\nPhilippe Blain (2):\n test-lib-functions: use user's SHELL, HOME and TERM for 'test_pause'\n test-lib-functions: use user's TERM and HOME for 'debug'\n\n t/test-lib-functions.sh | 4 ++--\n t/test-lib.sh | 6 ++++--\n 2 files changed, 6 insertions(+), 4 deletions(-)\n\n\nbase-commit: 225bc32a989d7a22fa6addafd4ce7dcd04675dbf\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1022%2Fphil-blain%2Ftest-pause-and-debug-easier-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1022/phil-blain/test-pause-and-debug-easier-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/1022\n-- \ngitgitgadget\n"},{"id":"433136","messageId":"bf916ad98ccc1526f5298d1c3e1740cabad093af.1629393395.git.gitgitgadget@gmail.com","threadId":"56325","inReplyTo":"pull.1022.git.1629393395.gitgitgadget@gmail.com","subject":"[PATCH 1/2] test-lib-functions: use user's SHELL, HOME and TERM for 'test_pause'","fromName":"Philippe Blain via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-08-19T17:16:34Z","receivedAt":"2021-08-19T17:16:46Z","isPatch":true,"sender":{"key":"levraiphilippeblain@gmail.com","avatar":"https://avatars.githubusercontent.com/u/44212482?v=4"},"body":"From: Philippe Blain \n\nThe 'test_pause' function, which is designed to help interactive\ndebugging and exploration of tests, currently inherits the value of HOME\nand TERM set by 'test-lib.sh': HOME=\"$TRASH_DIRECTORY\" and TERM=dumb. It\nalso invokes the shell defined by SHELL_PATH, which defaults to /bin/sh.\n\nChanging the value of HOME means that any customization configured in a\ndevelopers' shell startup files and any Git aliases defined in their\nglobal Git configuration file are not available in the shell invoked by\n'test_pause'.\n\nChanging the value of TERM to 'dumb' means that colored output\nis disabled for all commands in that shell.\n\nUsing /bin/sh as the shell invoked by 'test_pause' is not ideal since\nsome platforms (i.e. Debian and derivatives) use Dash as /bin/sh, and\nthis shell is usually compiled without readline support, which makes for\na poor interactive command line experience.\n\nTo make the interactive command line experience in the shell invoked by\n'test_pause' more pleasant, save the values of HOME and TERM in\nUSER_HOME and USER_TERM before changing them in test-lib.sh, and use\nthese variables to invoke the shell in 'test_pause'. Also, invoke SHELL\ninstead of SHELL_PATH, so that developer's interactive shell is used.\n\nSigned-off-by: Philippe Blain \n---\n t/test-lib-functions.sh | 2 +-\n t/test-lib.sh | 6 ++++--\n 2 files changed, 5 insertions(+), 3 deletions(-)\n\ndiff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh\nindex e28411bb75a..662cfc4c3e0 100644\n--- a/t/test-lib-functions.sh\n+++ b/t/test-lib-functions.sh\n@@ -139,7 +139,7 @@ test_tick () {\n # Be sure to remove all invocations of this command before submitting.\n \n test_pause () {\n-\t\"$SHELL_PATH\" <&6 >&5 2>&7\n+\tTERM=\"$USER_TERM\" HOME=\"$USER_HOME\" \"$SHELL\" <&6 >&5 2>&7\n }\n \n # Wrap git with a debugger. Adding this to a command can make it easier\ndiff --git a/t/test-lib.sh b/t/test-lib.sh\nindex abcfbed6d61..132618991e2 100644\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -585,8 +585,9 @@ else\n \t}\n fi\n \n+USER_TERM=\"$TERM\"\n TERM=dumb\n-export TERM\n+export TERM USER_TERM\n \n error () {\n \tsay_color error \"error: $*\"\n@@ -1380,9 +1381,10 @@ then\n fi\n \n # Last-minute variable setup\n+USER_HOME=\"$HOME\"\n HOME=\"$TRASH_DIRECTORY\"\n GNUPGHOME=\"$HOME/gnupg-home-not-used\"\n-export HOME GNUPGHOME\n+export HOME GNUPGHOME USER_HOME\n \n # Test repository\n rm -fr \"$TRASH_DIRECTORY\" || {\n-- \ngitgitgadget\n\n"},{"id":"433135","messageId":"d51d0db6e250c118b2d0f6eff571e1a7642da00e.1629393395.git.gitgitgadget@gmail.com","threadId":"56325","inReplyTo":"pull.1022.git.1629393395.gitgitgadget@gmail.com","subject":"[PATCH 2/2] test-lib-functions: use user's TERM and HOME for 'debug'","fromName":"Philippe Blain via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-08-19T17:16:35Z","receivedAt":"2021-08-19T17:16:48Z","isPatch":true,"sender":{"key":"levraiphilippeblain@gmail.com","avatar":"https://avatars.githubusercontent.com/u/44212482?v=4"},"body":"From: Philippe Blain \n\nThe 'debug' function in test-lib-functions.sh is used to invoke a\ndebugger at a specific line in a test. It inherits the value of HOME and\nTERM set by 'test-lib.sh': HOME=\"$TRASH_DIRECTORY\" and TERM=dumb.\n\nChanging the value of HOME means that any customization configured in a\ndevelopers' debugger configuration file (like $HOME/.gdbinit or\n$HOME/.lldbinit) are not available in the debugger invoked by\n'test_pause'.\n\nChanging the value of TERM to 'dumb' means that colored output\nis disabled in the debugger.\n\nTo make the debugging experience with 'debug' more pleasant, leverage\nthe variables USER_HOME and USER_TERM, added in the previous commit, to\nset HOME and TERM before invoking the debugger.\n\nSigned-off-by: Philippe Blain \n---\n t/test-lib-functions.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh\nindex 662cfc4c3e0..86680b1177d 100644\n--- a/t/test-lib-functions.sh\n+++ b/t/test-lib-functions.sh\n@@ -163,7 +163,7 @@ debug () {\n \t\tGIT_DEBUGGER=1\n \t\t;;\n \tesac &&\n-\tGIT_DEBUGGER=\"${GIT_DEBUGGER}\" \"$@\" <&6 >&5 2>&7\n+\tTERM=\"$USER_TERM\" HOME=\"$USER_HOME\" GIT_DEBUGGER=\"${GIT_DEBUGGER}\" \"$@\" <&6 >&5 2>&7\n }\n \n # Usage: test_commit [options] [ [ []]]\n-- \ngitgitgadget\n"},{"id":"433142","messageId":"CAPig+cT2FfaYBbsRMy7vxXkvsxn4BcFTn4bGJjgvJKznXg1OYg@mail.gmail.com","threadId":"56325","inReplyTo":"pull.1022.git.1629393395.gitgitgadget@gmail.com","subject":"Re: [PATCH 0/2] test-lib-functions.sh: keep user's HOME, TERM and SHELL for 'test_pause' and 'debug'","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2021-08-19T18:10:05Z","receivedAt":"2021-08-19T18:10:18Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Thu, Aug 19, 2021 at 1:16 PM Philippe Blain via GitGitGadget\n wrote:\n> This series proposes two small quality-of-life improvements (in my opinion)\n> to the 'test_pause' and 'debug' test functions: using the original values of\n> HOME and TERM (before they are changed by the test framework) and using\n> SHELL instead of SHELL_PATH.\n>\n> The later might be too big of a change, but I think it makes sense. We could\n> add a new GIT_TEST_* to conditionnaly change the behaviour, but I kept it\n> simple for v1.\n\nI also find the test_pause() user-experience suboptimal and appreciate\nthe idea of improving it. However, this approach seems fatally flawed.\nIn particular, setting HOME to the user's real home directory could\nlead to undesirable results. When I'm using test_pause() to debug a\nproblem with a test, I'm not just inspecting the test state, but I\nquite often interact with the state using the same Git commands as the\ntest itself would use. Hence, it is very common for me to pause the\ntest just before the suspect commands and then run those commands\nmanually (instead of allowing the test script to do so). In such a\nscenario, HOME must be pointing at the test's home directory, not at\nmy real home directory.\n\nPerhaps there's some way to achieve your goal for at least certain\nshells by detecting the shell and taking advantage of special shell\nfeatures which allow you to launch the shell and run some canned\ncommands to set up the shell as desired before dropping into an\ninteractive session[1] -- but it's just an idle thought.\n\n[1]: For Bash, for instance, a quick bit of Googling leads to:\nhttps://serverfault.com/a/586272\n"},{"id":"433152","messageId":"YR6v9yu3jpmRCZvy@nand.local","threadId":"56325","inReplyTo":"d51d0db6e250c118b2d0f6eff571e1a7642da00e.1629393395.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 2/2] test-lib-functions: use user's TERM and HOME for 'debug'","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2021-08-19T19:24:39Z","receivedAt":"2021-08-19T19:24:42Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Thu, Aug 19, 2021 at 05:16:35PM +0000, Philippe Blain via GitGitGadget wrote:\n> Signed-off-by: Philippe Blain \n> ---\n> t/test-lib-functions.sh | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh\n> index 662cfc4c3e0..86680b1177d 100644\n> --- a/t/test-lib-functions.sh\n> +++ b/t/test-lib-functions.sh\n> @@ -163,7 +163,7 @@ debug () {\n> \t\tGIT_DEBUGGER=1\n> \t\t;;\n> \tesac &&\n> -\tGIT_DEBUGGER=\"${GIT_DEBUGGER}\" \"$@\" <&6 >&5 2>&7\n> +\tTERM=\"$USER_TERM\" HOME=\"$USER_HOME\" GIT_DEBUGGER=\"${GIT_DEBUGGER}\" \"$@\" <&6 >&5 2>&7\n\nI also share some concerns about setting $HOME here (though less than in\ntest_pause), but forwarding $USER_TERM down would be so nice. I have a\nmuscle memory of 'tui enable' for anything besides absolutely trivial\ndebugging, and it's always so frustrating to see:\n\n (gdb) tui enable\n Cannot enable the TUI: terminal doesn't support cursor addressing [TERM=dumb]\n\nSo I would welcome even just that part of this change.\n\nThanks,\nTaylor\n"},{"id":"433156","messageId":"xmqqsfz5yzp5.fsf@gitster.g","threadId":"56325","inReplyTo":"CAPig+cT2FfaYBbsRMy7vxXkvsxn4BcFTn4bGJjgvJKznXg1OYg@mail.gmail.com","subject":"Re: [PATCH 0/2] test-lib-functions.sh: keep user's HOME, TERM and SHELL for 'test_pause' and 'debug'","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-08-19T19:57:58Z","receivedAt":"2021-08-19T19:58:11Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine writes:\n\n> I also find the test_pause() user-experience suboptimal and appreciate\n> the idea of improving it. However, this approach seems fatally flawed.\n> In particular, setting HOME to the user's real home directory could\n> lead to undesirable results. When I'm using test_pause() to debug a\n> problem with a test, I'm not just inspecting the test state, but I\n> quite often interact with the state using the same Git commands as the\n> test itself would use.\n\nYes, I do agree with you that it is a valid concern.\n\nI wonder if the developers can configure tools used during debugging\nsession with XDG so that HOME can be kept for the \"fake user that\nran the test suite, with the fake user's configuration\"?\n"},{"id":"433157","messageId":"CABPp-BFKEoHTgdSnUO86zSPYR0mLHv-vUB+Z-SoTtYQbUY4H=Q@mail.gmail.com","threadId":"56325","inReplyTo":"CAPig+cT2FfaYBbsRMy7vxXkvsxn4BcFTn4bGJjgvJKznXg1OYg@mail.gmail.com","subject":"Re: [PATCH 0/2] test-lib-functions.sh: keep user's HOME, TERM and SHELL for 'test_pause' and 'debug'","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2021-08-19T20:03:09Z","receivedAt":"2021-08-19T20:03:22Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Thu, Aug 19, 2021 at 11:10 AM Eric Sunshine wrote:\n>\n> On Thu, Aug 19, 2021 at 1:16 PM Philippe Blain via GitGitGadget\n> wrote:\n> > This series proposes two small quality-of-life improvements (in my opinion)\n> > to the 'test_pause' and 'debug' test functions: using the original values of\n> > HOME and TERM (before they are changed by the test framework) and using\n> > SHELL instead of SHELL_PATH.\n> >\n> > The later might be too big of a change, but I think it makes sense. We could\n> > add a new GIT_TEST_* to conditionnaly change the behaviour, but I kept it\n> > simple for v1.\n>\n> I also find the test_pause() user-experience suboptimal and appreciate\n> the idea of improving it. However, this approach seems fatally flawed.\n> In particular, setting HOME to the user's real home directory could\n> lead to undesirable results. When I'm using test_pause() to debug a\n> problem with a test, I'm not just inspecting the test state, but I\n> quite often interact with the state using the same Git commands as the\n> test itself would use. Hence, it is very common for me to pause the\n> test just before the suspect commands and then run those commands\n> manually (instead of allowing the test script to do so). In such a\n> scenario, HOME must be pointing at the test's home directory, not at\n> my real home directory.\n\nI agree, but I worry that it's not just HOME. I'd think the point of\ntest_pause is to let you interact with the repository state while\ngetting the same results that the test framework would, and I think\nsome tests could be affected by TERM and SHELL too (e.g. perhaps the\nrecent issues with COLUMNS). Granted, I suspect far fewer tests would\nbe affected by those, but I'm not sure I like the idea of inability to\nreproduce the same issues.\n\nMaybe we just need a different construct or special flags to test_pause?\n\n\n> Perhaps there's some way to achieve your goal for at least certain\n> shells by detecting the shell and taking advantage of special shell\n> features which allow you to launch the shell and run some canned\n> commands to set up the shell as desired before dropping into an\n> interactive session[1] -- but it's just an idle thought.\n>\n> [1]: For Bash, for instance, a quick bit of Googling leads to:\n> https://serverfault.com/a/586272\n"},{"id":"433163","messageId":"CAPig+cRk-BB79+pO83A6Qov9W75_91gzaHY4Le1MYKDUWagugg@mail.gmail.com","threadId":"56325","inReplyTo":"CABPp-BFKEoHTgdSnUO86zSPYR0mLHv-vUB+Z-SoTtYQbUY4H=Q@mail.gmail.com","subject":"Re: [PATCH 0/2] test-lib-functions.sh: keep user's HOME, TERM and SHELL for 'test_pause' and 'debug'","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2021-08-19T20:11:10Z","receivedAt":"2021-08-19T20:11:24Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Thu, Aug 19, 2021 at 4:03 PM Elijah Newren wrote:\n> On Thu, Aug 19, 2021 at 11:10 AM Eric Sunshine wrote:\n> > I also find the test_pause() user-experience suboptimal and appreciate\n> > the idea of improving it. However, this approach seems fatally flawed.\n> > In particular, setting HOME to the user's real home directory could\n> > lead to undesirable results. When I'm using test_pause() to debug a\n> > problem with a test, I'm not just inspecting the test state, but I\n> > quite often interact with the state using the same Git commands as the\n> > test itself would use. Hence, it is very common for me to pause the\n> > test just before the suspect commands and then run those commands\n> > manually (instead of allowing the test script to do so). In such a\n> > scenario, HOME must be pointing at the test's home directory, not at\n> > my real home directory.\n>\n> I agree, but I worry that it's not just HOME. I'd think the point of\n> test_pause is to let you interact with the repository state while\n> getting the same results that the test framework would, and I think\n> some tests could be affected by TERM and SHELL too (e.g. perhaps the\n> recent issues with COLUMNS). Granted, I suspect far fewer tests would\n> be affected by those, but I'm not sure I like the idea of inability to\n> reproduce the same issues.\n\nOh, indeed. I didn't mean to imply that HOME is the only problematic\none; they all are since, as you say, they can impact correctness and\nreproducibility of the tests themselves. I called out HOME specially\nbecause of the potential danger involved with pointing it at the\nuser's real home directory since it could very well lead to clobbering\nof precious files and other settings belonging to the user.\n"},{"id":"433164","messageId":"CAPig+cS-7nT7NHP4ALpoUOsxhwEJgcKgbLhd1BkAZSo=pBvArw@mail.gmail.com","threadId":"56325","inReplyTo":"xmqqsfz5yzp5.fsf@gitster.g","subject":"Re: [PATCH 0/2] test-lib-functions.sh: keep user's HOME, TERM and SHELL for 'test_pause' and 'debug'","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2021-08-19T20:14:29Z","receivedAt":"2021-08-19T20:14:43Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Thu, Aug 19, 2021 at 3:58 PM Junio C Hamano wrote:\n> Eric Sunshine writes:\n> > I also find the test_pause() user-experience suboptimal and appreciate\n> > the idea of improving it. However, this approach seems fatally flawed.\n> > In particular, setting HOME to the user's real home directory could\n> > lead to undesirable results. When I'm using test_pause() to debug a\n> > problem with a test, I'm not just inspecting the test state, but I\n> > quite often interact with the state using the same Git commands as the\n> > test itself would use.\n>\n> Yes, I do agree with you that it is a valid concern.\n>\n> I wonder if the developers can configure tools used during debugging\n> session with XDG so that HOME can be kept for the \"fake user that\n> ran the test suite, with the fake user's configuration\"?\n\nI haven't studied XDG deeply enough to answer, though it would not\nhelp macOS or Windows users (but that's not an argument against\npursuing an XDG solution -- it might still help some developers).\n"},{"id":"433198","messageId":"CAPUEspiu35JZZ1z6BosGyWeE9z4uOBhSotFL6iF+jEY8tXsjhw@mail.gmail.com","threadId":"56325","inReplyTo":"bf916ad98ccc1526f5298d1c3e1740cabad093af.1629393395.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 1/2] test-lib-functions: use user's SHELL, HOME and TERM for 'test_pause'","fromName":"Carlo Arenas","fromEmail":"carenas@gmail.com","sentAt":"2021-08-20T03:08:32Z","receivedAt":"2021-08-20T03:08:53Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"On Thu, Aug 19, 2021 at 10:17 AM Philippe Blain via GitGitGadget\n wrote:\n>\n> From: Philippe Blain \n>\n> The 'test_pause' function, which is designed to help interactive\n> debugging and exploration of tests, currently inherits the value of HOME\n> and TERM set by 'test-lib.sh': HOME=\"$TRASH_DIRECTORY\" and TERM=dumb. It\n> also invokes the shell defined by SHELL_PATH, which defaults to /bin/sh.\n\nthat is a bug, it should have been TEST_SHELL_PATH instead.\n\ngoes without saying, that if you don't really need that shell for your\ninteractive session, nothing prevents you from calling bash and\nresetting TERM or even HOME as needed\n\nCarlo\n"},{"id":"433199","messageId":"CAPUEsph3mAVWjJScpMrHObqAbmDnecc_e6K04cC2B6Q1NKqUzQ@mail.gmail.com","threadId":"56325","inReplyTo":"YR6v9yu3jpmRCZvy@nand.local","subject":"Re: [PATCH 2/2] test-lib-functions: use user's TERM and HOME for 'debug'","fromName":"Carlo Arenas","fromEmail":"carenas@gmail.com","sentAt":"2021-08-20T03:18:53Z","receivedAt":"2021-08-20T03:19:15Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"On Thu, Aug 19, 2021 at 12:25 PM Taylor Blau wrote:\n>\n> On Thu, Aug 19, 2021 at 05:16:35PM +0000, Philippe Blain via GitGitGadget wrote:\n> > Signed-off-by: Philippe Blain \n> > ---\n> > t/test-lib-functions.sh | 2 +-\n> > 1 file changed, 1 insertion(+), 1 deletion(-)\n> >\n> > diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh\n> > index 662cfc4c3e0..86680b1177d 100644\n> > --- a/t/test-lib-functions.sh\n> > +++ b/t/test-lib-functions.sh\n> > @@ -163,7 +163,7 @@ debug () {\n> > GIT_DEBUGGER=1\n> > ;;\n> > esac &&\n> > - GIT_DEBUGGER=\"${GIT_DEBUGGER}\" \"$@\" <&6 >&5 2>&7\n> > + TERM=\"$USER_TERM\" HOME=\"$USER_HOME\" GIT_DEBUGGER=\"${GIT_DEBUGGER}\" \"$@\" <&6 >&5 2>&7\n>\n> I also share some concerns about setting $HOME here (though less than in\n> test_pause)\n\ninstead of changing $HOME the needed dot files could be as well linked\ninto the current $HOME.\nthis will of course need extra code and specific knowledge of the\ndebugger that was invoked but will be IMHO safer and accomplish the\nsame objective.\n\nCarlo\n\nPS. I remember once wondering what GIT_DEBUGGER=1 meant and how it was\nmeant to be used, AFAIK that and the first use case were always broken\notherwise, maybe someone who knows and uses this code better could\nchime in.\n"},{"id":"433216","messageId":"53752b7d-c294-9507-a7f0-af2aee491251@gmail.com","threadId":"56325","inReplyTo":"CAPig+cRk-BB79+pO83A6Qov9W75_91gzaHY4Le1MYKDUWagugg@mail.gmail.com","subject":"Re: [PATCH 0/2] test-lib-functions.sh: keep user's HOME, TERM and SHELL for 'test_pause' and 'debug'","fromName":"Philippe Blain","fromEmail":"levraiphilippeblain@gmail.com","sentAt":"2021-08-20T12:12:50Z","receivedAt":"2021-08-20T12:12:54Z","isPatch":true,"sender":{"key":"levraiphilippeblain@gmail.com","avatar":"https://avatars.githubusercontent.com/u/44212482?v=4"},"body":"Hi everyone,\n\nLe 2021-08-19 à 16:11, Eric Sunshine a écrit :\n> On Thu, Aug 19, 2021 at 4:03 PM Elijah Newren wrote:\n>> On Thu, Aug 19, 2021 at 11:10 AM Eric Sunshine wrote:\n\n>>> In such a\n>>> scenario, HOME must be pointing at the test's home directory, not at\n>>> my real home directory.\n>>\n>> I agree, but I worry that it's not just HOME. I'd think the point of\n>> test_pause is to let you interact with the repository state while\n>> getting the same results that the test framework would, and I think\n>> some tests could be affected by TERM and SHELL too (e.g. perhaps the\n>> recent issues with COLUMNS). Granted, I suspect far fewer tests would\n>> be affected by those, but I'm not sure I like the idea of inability to\n>> reproduce the same issues.\n> \n> Oh, indeed. I didn't mean to imply that HOME is the only problematic\n> one; they all are since, as you say, they can impact correctness and\n> reproducibility of the tests themselves. I called out HOME specially\n> because of the potential danger involved with pointing it at the\n> user's real home directory since it could very well lead to clobbering\n> of precious files and other settings belonging to the user.\n> \n\nThanks everyone for sharing their input and concerns. I understand that the behaviour\nchange might not be wanted all the time, or by everyone.\n\nI also did not think about the implications of changing $HOME that could lead to the\ntest framework overwriting stuff in my home. I checked the tests and there are only\na handful of them that seem to reference HOME, but still, for those tests it would be\nundesirable to reset HOME.\n\nIn light of this I'm thinking of simply adding flags to 'test_pause' and 'debug' to signal\nthat one wants to use their original TERM, HOME and SHELL, with appropriate caveats in\nthe description of the functions:\n\ntest_pause # original behaviour\ntest_pause -t # use USER_TERM\ntest_pause -s # use SHELL instead of TEST_SHELL_PATH\ntest_pause -h # use USER_HOME\n\nand combinations of these three.\n\nFor 'debug', Carlo's idea of just symlinking/copying gdbinit and/or llldbinit to the test\nHOME might be easier, and would cover the majority of developers, I think. As for TERM,\nwe could do 'debug -t' as above, or use USER_TERM always...\n\nI'll explore these ideas before sending v2.\n\nThanks,\n\nPhilippe.\n"},{"id":"433217","messageId":"9b563662-6208-26e9-ff84-d10b98078cd4@gmail.com","threadId":"56325","inReplyTo":"CAPUEspiu35JZZ1z6BosGyWeE9z4uOBhSotFL6iF+jEY8tXsjhw@mail.gmail.com","subject":"Re: [PATCH 1/2] test-lib-functions: use user's SHELL, HOME and TERM for 'test_pause'","fromName":"Philippe Blain","fromEmail":"levraiphilippeblain@gmail.com","sentAt":"2021-08-20T12:14:50Z","receivedAt":"2021-08-20T12:14:55Z","isPatch":true,"sender":{"key":"levraiphilippeblain@gmail.com","avatar":"https://avatars.githubusercontent.com/u/44212482?v=4"},"body":"Hi Carlo,\n\nLe 2021-08-19 à 23:08, Carlo Arenas a écrit :\n> On Thu, Aug 19, 2021 at 10:17 AM Philippe Blain via GitGitGadget\n> wrote:\n>>\n>> From: Philippe Blain \n>>\n>> The 'test_pause' function, which is designed to help interactive\n>> debugging and exploration of tests, currently inherits the value of HOME\n>> and TERM set by 'test-lib.sh': HOME=\"$TRASH_DIRECTORY\" and TERM=dumb. It\n>> also invokes the shell defined by SHELL_PATH, which defaults to /bin/sh.\n> \n> that is a bug, it should have been TEST_SHELL_PATH instead.\n\nRight. I'll make that change unconditionnally as a preparatory step.\n\n> \n> goes without saying, that if you don't really need that shell for your\n> interactive session, nothing prevents you from calling bash and\n> resetting TERM or even HOME as needed\n\nYes. I'm just trying to streamline to experience :)\n"},{"id":"433238","messageId":"CAPig+cR7HhWN8F1G7_rWpD_jXfaKqaj7c4fMbkgAGvcjf1pX6g@mail.gmail.com","threadId":"56325","inReplyTo":"53752b7d-c294-9507-a7f0-af2aee491251@gmail.com","subject":"Re: [PATCH 0/2] test-lib-functions.sh: keep user's HOME, TERM and SHELL for 'test_pause' and 'debug'","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2021-08-20T15:50:09Z","receivedAt":"2021-08-20T15:50:23Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Fri, Aug 20, 2021 at 8:12 AM Philippe Blain\n wrote:\n> Le 2021-08-19 à 16:11, Eric Sunshine a écrit :\n> > Oh, indeed. I didn't mean to imply that HOME is the only problematic\n> > one; they all are since, as you say, they can impact correctness and\n> > reproducibility of the tests themselves. I called out HOME specially\n> > because of the potential danger involved with pointing it at the\n> > user's real home directory since it could very well lead to clobbering\n> > of precious files and other settings belonging to the user.\n>\n> I also did not think about the implications of changing $HOME that could lead to the\n> test framework overwriting stuff in my home. I checked the tests and there are only\n> a handful of them that seem to reference HOME, but still, for those tests it would be\n> undesirable to reset HOME.\n\nIt's not just tests which reference HOME explicitly which are\nproblematic. Git commands themselves access files and configuration\npointed at by HOME. Worse, Git commands invoked by tests can alter\nconfiguration, so the potential for damage is wider than the few tests\nwhich reference HOME explicitly.\n\n> In light of this I'm thinking of simply adding flags to 'test_pause' and 'debug' to signal\n> that one wants to use their original TERM, HOME and SHELL, with appropriate caveats in\n> the description of the functions:\n>\n> test_pause # original behaviour\n> test_pause -t # use USER_TERM\n> test_pause -s # use SHELL instead of TEST_SHELL_PATH\n> test_pause -h # use USER_HOME\n\nA (very) stray thought I had: Rather than mucking with the environment\nvariables -- which can impact test reproducibility and correctness and\ncan potentially damage the user's precious files and configuration --\nanother possibility might be to detect which shell is being used\n(whether it be bash, zsh, etc.), and then enable certain useful\noptions specific to the shell, such as tab-completion, colors, etc.\nThis approach doesn't give developers the customized shell experience\nthey're used to (it wouldn't have their aliases or configuration, for\ninstance), but it at least might make the test_pause() experience a\nbit more pleasant.\n"},{"id":"433268","messageId":"YR/zCcPF2FTeBXvH@coredump.intra.peff.net","threadId":"56325","inReplyTo":"53752b7d-c294-9507-a7f0-af2aee491251@gmail.com","subject":"Re: [PATCH 0/2] test-lib-functions.sh: keep user's HOME, TERM and SHELL for 'test_pause' and 'debug'","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-08-20T18:23:05Z","receivedAt":"2021-08-20T18:23:22Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Aug 20, 2021 at 08:12:50AM -0400, Philippe Blain wrote:\n\n> Thanks everyone for sharing their input and concerns. I understand that the behaviour\n> change might not be wanted all the time, or by everyone.\n> \n> I also did not think about the implications of changing $HOME that could lead to the\n> test framework overwriting stuff in my home. I checked the tests and there are only\n> a handful of them that seem to reference HOME, but still, for those tests it would be\n> undesirable to reset HOME.\n> \n> In light of this I'm thinking of simply adding flags to 'test_pause' and 'debug' to signal\n> that one wants to use their original TERM, HOME and SHELL, with appropriate caveats in\n> the description of the functions:\n> \n> test_pause # original behaviour\n> test_pause -t # use USER_TERM\n> test_pause -s # use SHELL instead of TEST_SHELL_PATH\n> test_pause -h # use USER_HOME\n> \n> and combinations of these three.\n> \n> For 'debug', Carlo's idea of just symlinking/copying gdbinit and/or llldbinit to the test\n> HOME might be easier, and would cover the majority of developers, I think. As for TERM,\n> we could do 'debug -t' as above, or use USER_TERM always...\n> \n> I'll explore these ideas before sending v2.\n\nAs an even more different alternative, what do you think about making it\neasier to break out of the test suite at a particular state? That would\nlet you inspect it from your regular shell environment.\n\nE.g., most of my test debugging happens via:\n\n ./t1234-foo.sh -i\n cd trash\\ directory-1234-foo/\n gdb --args ../../git some-command\n\nThat implies that the test you're interested in is actually failing\n(though I have been known to insert \"false &&\" to make it so), and that\nrunning the test doesn't wreck the on-disk state.\n\nBut what if we could do something like:\n\n ./t1234-foo.sh --debug=42\n\nto stop _before_ running test 42, print out the commands it would run,\nand leave the trash directory so that you can inspect it yourself?\n\nThat does not have the \"resume after I'm done inspecting\" property that\ntest_pause, but IMHO that is not really that useful for debugging.\n\nIt will also occasionally cause headaches when a test relies on other\nparts of the environment (e.g., environment variables defined\npreviously, or functions defined in the script). But I find those are\nusually a minority of cases.\n\n-Peff\n"},{"id":"433990","messageId":"pull.1022.v2.git.1630111653.gitgitgadget@gmail.com","threadId":"56325","inReplyTo":"pull.1022.git.1629393395.gitgitgadget@gmail.com","subject":"[PATCH v2 0/3] test-lib-functions.sh: keep user's HOME, TERM and SHELL for 'test_pause' and 'debug'","fromName":"Philippe Blain via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-08-28T00:47:30Z","receivedAt":"2021-08-28T00:47:37Z","isPatch":true,"sender":{"key":"levraiphilippeblain@gmail.com","avatar":"https://avatars.githubusercontent.com/u/44212482?v=4"},"body":"Changes since v1:\n\n * added 1/3 as a preliminary step to use TEST_SHELL_PATH in test_pause\n instead of SHELL_PATH, as suggested by Carlos\n * implemented the change in behaviour through optional flags in both\n test_pause and debug. This seemed to be the simplest way to keep the\n current behaviour but also provide a way to improve the UX.\n\nv1: This series proposes two small quality-of-life improvements (in my\nopinion) to the 'test_pause' and 'debug' test functions: using the original\nvalues of HOME and TERM (before they are changed by the test framework) and\nusing SHELL instead of SHELL_PATH.\n\nThe later might be too big of a change, but I think it makes sense. We could\nadd a new GIT_TEST_* to conditionnaly change the behaviour, but I kept it\nsimple for v1.\n\nCheers, Philippe.\n\nPhilippe Blain (3):\n test-lib-functions: use 'TEST_SHELL_PATH' in 'test_pause'\n test-lib-functions: optionally keep HOME, TERM and SHELL in\n 'test_pause'\n test-lib-functions: optionally keep HOME and TERM in 'debug'\n\n t/test-lib-functions.sh | 91 ++++++++++++++++++++++++++++++++++-------\n t/test-lib.sh | 6 ++-\n 2 files changed, 80 insertions(+), 17 deletions(-)\n\n\nbase-commit: 225bc32a989d7a22fa6addafd4ce7dcd04675dbf\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1022%2Fphil-blain%2Ftest-pause-and-debug-easier-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1022/phil-blain/test-pause-and-debug-easier-v2\nPull-Request: https://github.com/gitgitgadget/git/pull/1022\n\nRange-diff vs v1:\n\n -: ----------- > 1: 2f566f330e0 test-lib-functions: use 'TEST_SHELL_PATH' in 'test_pause'\n 1: bf916ad98cc ! 2: 00211457ece test-lib-functions: use user's SHELL, HOME and TERM for 'test_pause'\n @@ Metadata\n Author: Philippe Blain \n \n ## Commit message ##\n - test-lib-functions: use user's SHELL, HOME and TERM for 'test_pause'\n + test-lib-functions: optionally keep HOME, TERM and SHELL in 'test_pause'\n \n The 'test_pause' function, which is designed to help interactive\n debugging and exploration of tests, currently inherits the value of HOME\n and TERM set by 'test-lib.sh': HOME=\"$TRASH_DIRECTORY\" and TERM=dumb. It\n - also invokes the shell defined by SHELL_PATH, which defaults to /bin/sh.\n + also invokes the shell defined by TEST_SHELL_PATH, which defaults to\n + /bin/sh (through SHELL_PATH).\n \n Changing the value of HOME means that any customization configured in a\n developers' shell startup files and any Git aliases defined in their\n @@ Commit message\n \n To make the interactive command line experience in the shell invoked by\n 'test_pause' more pleasant, save the values of HOME and TERM in\n - USER_HOME and USER_TERM before changing them in test-lib.sh, and use\n - these variables to invoke the shell in 'test_pause'. Also, invoke SHELL\n - instead of SHELL_PATH, so that developer's interactive shell is used.\n + USER_HOME and USER_TERM before changing them in test-lib.sh, and add\n + options to 'test_pause' to optionally use these variables to invoke the\n + shell. Also add an option to invoke SHELL instead of TEST_SHELL_PATH, so\n + that developer's interactive shell is used.\n +\n + We use options instead of changing the behaviour unconditionally since\n + these three variables can break test reproducibility. Moreover, using the\n + original HOME means tests could overwrite files in a user's home\n + directory. Be explicit about these caveats in the new 'Usage' section in\n + test-lib-functions.sh.\n \n Signed-off-by: Philippe Blain \n \n ## t/test-lib-functions.sh ##\n @@ t/test-lib-functions.sh: test_tick () {\n + # Stop execution and start a shell. This is useful for debugging tests.\n + #\n # Be sure to remove all invocations of this command before submitting.\n ++#\n ++# Usage: test_pause [options]\n ++# -t\n ++#\tUse your original TERM instead of test-lib.sh's \"dumb\".\n ++#\tThis usually restores color output in the invoked shell.\n ++#\tWARNING: this can break test reproducibility.\n ++# -s\n ++#\tInvoke $SHELL instead of $TEST_SHELL_PATH\n ++#\tWARNING: this can break test reproducibility.\n ++# -h\n ++#\tUse your original HOME instead of test-lib.sh's \"$TRASH_DIRECTORY\".\n ++#\tThis allows you to use your regular shell environment and Git aliases.\n ++#\tWARNING: this can break test reproducibility.\n ++#\tCAUTION: this can overwrite files in your HOME.\n \n test_pause () {\n --\t\"$SHELL_PATH\" <&6 >&5 2>&7\n -+\tTERM=\"$USER_TERM\" HOME=\"$USER_HOME\" \"$SHELL\" <&6 >&5 2>&7\n +-\t\"$TEST_SHELL_PATH\" <&6 >&5 2>&7\n ++\tPAUSE_TERM=$TERM &&\n ++\tPAUSE_SHELL=$TEST_SHELL_PATH &&\n ++\tPAUSE_HOME=$HOME &&\n ++\twhile test $# != 0\n ++\tdo\n ++\t\tcase \"$1\" in\n ++\t\t-t)\n ++\t\t\tPAUSE_TERM=\"$USER_TERM\"\n ++\t\t\t;;\n ++\t\t-s)\n ++\t\t\tPAUSE_SHELL=\"$SHELL\"\n ++\t\t\t;;\n ++\t\t-h)\n ++\t\t\tPAUSE_HOME=\"$USER_HOME\"\n ++\t\t\t;;\n ++\t\t*)\n ++\t\t\tbreak\n ++\t\t\t;;\n ++\t\tesac\n ++\t\tshift\n ++\tdone &&\n ++\tTERM=\"$PAUSE_TERM\" HOME=\"$PAUSE_HOME\" \"$PAUSE_SHELL\" <&6 >&5 2>&7\n }\n \n # Wrap git with a debugger. Adding this to a command can make it easier\n 2: d51d0db6e25 < -: ----------- test-lib-functions: use user's TERM and HOME for 'debug'\n -: ----------- > 3: 1fac9baec1d test-lib-functions: optionally keep HOME and TERM in 'debug'\n\n-- \ngitgitgadget\n"},{"id":"433991","messageId":"2f566f330e01ed4249edad54671e98301dbeec5a.1630111653.git.gitgitgadget@gmail.com","threadId":"56325","inReplyTo":"pull.1022.v2.git.1630111653.gitgitgadget@gmail.com","subject":"[PATCH v2 1/3] test-lib-functions: use 'TEST_SHELL_PATH' in 'test_pause'","fromName":"Philippe Blain via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-08-28T00:47:31Z","receivedAt":"2021-08-28T00:47:39Z","isPatch":true,"sender":{"key":"levraiphilippeblain@gmail.com","avatar":"https://avatars.githubusercontent.com/u/44212482?v=4"},"body":"From: Philippe Blain \n\n3f824e91c8 (t/Makefile: introduce TEST_SHELL_PATH, 2017-12-08)\nmade it easy to use a different shell for the tests than 'SHELL_PATH'\nused at compile time. But 'test_pause' still invokes 'SHELL_PATH'.\n\nIf TEST_SHELL_PATH is set, invoke that shell in 'test_pause' for\nconsistency.\n\nSuggested-by: Carlo Marcelo Arenas Belón \nSigned-off-by: Philippe Blain \n---\n t/test-lib-functions.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh\nindex e28411bb75a..1884177e293 100644\n--- a/t/test-lib-functions.sh\n+++ b/t/test-lib-functions.sh\n@@ -139,7 +139,7 @@ test_tick () {\n # Be sure to remove all invocations of this command before submitting.\n \n test_pause () {\n-\t\"$SHELL_PATH\" <&6 >&5 2>&7\n+\t\"$TEST_SHELL_PATH\" <&6 >&5 2>&7\n }\n \n # Wrap git with a debugger. Adding this to a command can make it easier\n-- \ngitgitgadget\n\n"},{"id":"433993","messageId":"00211457eceab756528abf0ca296af866cda00e7.1630111653.git.gitgitgadget@gmail.com","threadId":"56325","inReplyTo":"pull.1022.v2.git.1630111653.gitgitgadget@gmail.com","subject":"[PATCH v2 2/3] test-lib-functions: optionally keep HOME, TERM and SHELL in 'test_pause'","fromName":"Philippe Blain via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-08-28T00:47:32Z","receivedAt":"2021-08-28T00:47:40Z","isPatch":true,"sender":{"key":"levraiphilippeblain@gmail.com","avatar":"https://avatars.githubusercontent.com/u/44212482?v=4"},"body":"From: Philippe Blain \n\nThe 'test_pause' function, which is designed to help interactive\ndebugging and exploration of tests, currently inherits the value of HOME\nand TERM set by 'test-lib.sh': HOME=\"$TRASH_DIRECTORY\" and TERM=dumb. It\nalso invokes the shell defined by TEST_SHELL_PATH, which defaults to\n/bin/sh (through SHELL_PATH).\n\nChanging the value of HOME means that any customization configured in a\ndevelopers' shell startup files and any Git aliases defined in their\nglobal Git configuration file are not available in the shell invoked by\n'test_pause'.\n\nChanging the value of TERM to 'dumb' means that colored output\nis disabled for all commands in that shell.\n\nUsing /bin/sh as the shell invoked by 'test_pause' is not ideal since\nsome platforms (i.e. Debian and derivatives) use Dash as /bin/sh, and\nthis shell is usually compiled without readline support, which makes for\na poor interactive command line experience.\n\nTo make the interactive command line experience in the shell invoked by\n'test_pause' more pleasant, save the values of HOME and TERM in\nUSER_HOME and USER_TERM before changing them in test-lib.sh, and add\noptions to 'test_pause' to optionally use these variables to invoke the\nshell. Also add an option to invoke SHELL instead of TEST_SHELL_PATH, so\nthat developer's interactive shell is used.\n\nWe use options instead of changing the behaviour unconditionally since\nthese three variables can break test reproducibility. Moreover, using the\noriginal HOME means tests could overwrite files in a user's home\ndirectory. Be explicit about these caveats in the new 'Usage' section in\ntest-lib-functions.sh.\n\nSigned-off-by: Philippe Blain \n---\n t/test-lib-functions.sh | 37 ++++++++++++++++++++++++++++++++++++-\n t/test-lib.sh | 6 ++++--\n 2 files changed, 40 insertions(+), 3 deletions(-)\n\ndiff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh\nindex 1884177e293..1b775343adc 100644\n--- a/t/test-lib-functions.sh\n+++ b/t/test-lib-functions.sh\n@@ -137,9 +137,44 @@ test_tick () {\n # Stop execution and start a shell. This is useful for debugging tests.\n #\n # Be sure to remove all invocations of this command before submitting.\n+#\n+# Usage: test_pause [options]\n+# -t\n+#\tUse your original TERM instead of test-lib.sh's \"dumb\".\n+#\tThis usually restores color output in the invoked shell.\n+#\tWARNING: this can break test reproducibility.\n+# -s\n+#\tInvoke $SHELL instead of $TEST_SHELL_PATH\n+#\tWARNING: this can break test reproducibility.\n+# -h\n+#\tUse your original HOME instead of test-lib.sh's \"$TRASH_DIRECTORY\".\n+#\tThis allows you to use your regular shell environment and Git aliases.\n+#\tWARNING: this can break test reproducibility.\n+#\tCAUTION: this can overwrite files in your HOME.\n \n test_pause () {\n-\t\"$TEST_SHELL_PATH\" <&6 >&5 2>&7\n+\tPAUSE_TERM=$TERM &&\n+\tPAUSE_SHELL=$TEST_SHELL_PATH &&\n+\tPAUSE_HOME=$HOME &&\n+\twhile test $# != 0\n+\tdo\n+\t\tcase \"$1\" in\n+\t\t-t)\n+\t\t\tPAUSE_TERM=\"$USER_TERM\"\n+\t\t\t;;\n+\t\t-s)\n+\t\t\tPAUSE_SHELL=\"$SHELL\"\n+\t\t\t;;\n+\t\t-h)\n+\t\t\tPAUSE_HOME=\"$USER_HOME\"\n+\t\t\t;;\n+\t\t*)\n+\t\t\tbreak\n+\t\t\t;;\n+\t\tesac\n+\t\tshift\n+\tdone &&\n+\tTERM=\"$PAUSE_TERM\" HOME=\"$PAUSE_HOME\" \"$PAUSE_SHELL\" <&6 >&5 2>&7\n }\n \n # Wrap git with a debugger. Adding this to a command can make it easier\ndiff --git a/t/test-lib.sh b/t/test-lib.sh\nindex abcfbed6d61..132618991e2 100644\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -585,8 +585,9 @@ else\n \t}\n fi\n \n+USER_TERM=\"$TERM\"\n TERM=dumb\n-export TERM\n+export TERM USER_TERM\n \n error () {\n \tsay_color error \"error: $*\"\n@@ -1380,9 +1381,10 @@ then\n fi\n \n # Last-minute variable setup\n+USER_HOME=\"$HOME\"\n HOME=\"$TRASH_DIRECTORY\"\n GNUPGHOME=\"$HOME/gnupg-home-not-used\"\n-export HOME GNUPGHOME\n+export HOME GNUPGHOME USER_HOME\n \n # Test repository\n rm -fr \"$TRASH_DIRECTORY\" || {\n-- \ngitgitgadget\n\n"},{"id":"433992","messageId":"1fac9baec1d5b9d42d925c1e6d0005a396c64164.1630111653.git.gitgitgadget@gmail.com","threadId":"56325","inReplyTo":"pull.1022.v2.git.1630111653.gitgitgadget@gmail.com","subject":"[PATCH v2 3/3] test-lib-functions: optionally keep HOME and TERM in 'debug'","fromName":"Philippe Blain via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-08-28T00:47:33Z","receivedAt":"2021-08-28T00:47:41Z","isPatch":true,"sender":{"key":"levraiphilippeblain@gmail.com","avatar":"https://avatars.githubusercontent.com/u/44212482?v=4"},"body":"From: Philippe Blain \n\nThe 'debug' function in test-lib-functions.sh is used to invoke a\ndebugger at a specific line in a test. It inherits the value of HOME and\nTERM set by 'test-lib.sh': HOME=\"$TRASH_DIRECTORY\" and TERM=dumb.\n\nChanging the value of HOME means that any customization configured in a\ndevelopers' debugger configuration file (like $HOME/.gdbinit or\n$HOME/.lldbinit) are not available in the debugger invoked by\n'test_pause'.\n\nChanging the value of TERM to 'dumb' means that colored output\nis disabled in the debugger.\n\nTo make the debugging experience with 'debug' more pleasant, leverage\nthe variables USER_HOME and USER_TERM, added in the previous commit, to\noptionally set HOME and TERM before invoking the debugger.\n\nAdd the same warnings as for 'test_pause' in the \"Usage\" section.\n\nSigned-off-by: Philippe Blain \n---\n t/test-lib-functions.sh | 54 ++++++++++++++++++++++++++++++-----------\n 1 file changed, 40 insertions(+), 14 deletions(-)\n\ndiff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh\nindex 1b775343adc..0f6d30e2b3f 100644\n--- a/t/test-lib-functions.sh\n+++ b/t/test-lib-functions.sh\n@@ -180,25 +180,51 @@ test_pause () {\n # Wrap git with a debugger. Adding this to a command can make it easier\n # to understand what is going on in a failing test.\n #\n+# Usage: debug [options] [git command]\n+# -d \n+# --debugger=\n+#\tUse instead of GDB\n+# -t\n+#\tUse your original TERM instead of test-lib.sh's \"dumb\".\n+#\tThis usually restores color output in the debugger.\n+#\tWARNING: this can break test reproducibility.\n+# -h\n+#\tUse your original HOME instead of test-lib.sh's \"$TRASH_DIRECTORY\".\n+#\tThis allows your debugger to find its config file in your home.\n+#\tWARNING: this can break test reproducibility.\n+#\tCAUTION: this can overwrite files in your HOME.\n+#\n # Examples:\n # debug git checkout master\n # debug --debugger=nemiver git $ARGS\n # debug -d \"valgrind --tool=memcheck --track-origins=yes\" git $ARGS\n debug () {\n-\tcase \"$1\" in\n-\t-d)\n-\t\tGIT_DEBUGGER=\"$2\" &&\n-\t\tshift 2\n-\t\t;;\n-\t--debugger=*)\n-\t\tGIT_DEBUGGER=\"${1#*=}\" &&\n-\t\tshift 1\n-\t\t;;\n-\t*)\n-\t\tGIT_DEBUGGER=1\n-\t\t;;\n-\tesac &&\n-\tGIT_DEBUGGER=\"${GIT_DEBUGGER}\" \"$@\" <&6 >&5 2>&7\n+\tGIT_DEBUGGER=1 &&\n+\tDEBUG_TERM=$TERM &&\n+\tDEBUG_HOME=$HOME &&\n+\twhile test $# != 0\n+\tdo\n+\t\tcase \"$1\" in\n+\t\t-t)\n+\t\t\tDEBUG_TERM=\"$USER_TERM\"\n+\t\t\t;;\n+\t\t-h)\n+\t\t\tDEBUG_HOME=\"$USER_HOME\"\n+\t\t\t;;\n+\t\t-d)\n+\t\t\tGIT_DEBUGGER=\"$2\" &&\n+\t\t\tshift\n+\t\t\t;;\n+\t\t--debugger=*)\n+\t\t\tGIT_DEBUGGER=\"${1#*=}\"\n+\t\t\t;;\n+\t\t*)\n+\t\t\tbreak\n+\t\t\t;;\n+\t\tesac\n+\t\tshift\n+\tdone &&\n+\tTERM=\"$DEBUG_TERM\" HOME=\"$DEBUG_HOME\" GIT_DEBUGGER=\"${GIT_DEBUGGER}\" \"$@\" <&6 >&5 2>&7\n }\n \n # Usage: test_commit [options] [ [ []]]\n-- \ngitgitgadget\n"},{"id":"434012","messageId":"CABPp-BFxDxD0Y5Ru+4VqebAJve4tjZk-4csN3JurC6hmRAUf4A@mail.gmail.com","threadId":"56325","inReplyTo":"00211457eceab756528abf0ca296af866cda00e7.1630111653.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 2/3] test-lib-functions: optionally keep HOME, TERM and SHELL in 'test_pause'","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2021-08-28T07:27:41Z","receivedAt":"2021-08-28T07:27:54Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Fri, Aug 27, 2021 at 5:47 PM Philippe Blain via GitGitGadget\n wrote:\n>\n> From: Philippe Blain \n>\n> The 'test_pause' function, which is designed to help interactive\n> debugging and exploration of tests, currently inherits the value of HOME\n> and TERM set by 'test-lib.sh': HOME=\"$TRASH_DIRECTORY\" and TERM=dumb. It\n> also invokes the shell defined by TEST_SHELL_PATH, which defaults to\n> /bin/sh (through SHELL_PATH).\n>\n> Changing the value of HOME means that any customization configured in a\n> developers' shell startup files and any Git aliases defined in their\n> global Git configuration file are not available in the shell invoked by\n> 'test_pause'.\n>\n> Changing the value of TERM to 'dumb' means that colored output\n> is disabled for all commands in that shell.\n>\n> Using /bin/sh as the shell invoked by 'test_pause' is not ideal since\n> some platforms (i.e. Debian and derivatives) use Dash as /bin/sh, and\n> this shell is usually compiled without readline support, which makes for\n> a poor interactive command line experience.\n>\n> To make the interactive command line experience in the shell invoked by\n> 'test_pause' more pleasant, save the values of HOME and TERM in\n> USER_HOME and USER_TERM before changing them in test-lib.sh, and add\n> options to 'test_pause' to optionally use these variables to invoke the\n> shell. Also add an option to invoke SHELL instead of TEST_SHELL_PATH, so\n> that developer's interactive shell is used.\n>\n> We use options instead of changing the behaviour unconditionally since\n> these three variables can break test reproducibility. Moreover, using the\n> original HOME means tests could overwrite files in a user's home\n> directory. Be explicit about these caveats in the new 'Usage' section in\n> test-lib-functions.sh.\n>\n> Signed-off-by: Philippe Blain \n> ---\n> t/test-lib-functions.sh | 37 ++++++++++++++++++++++++++++++++++++-\n> t/test-lib.sh | 6 ++++--\n> 2 files changed, 40 insertions(+), 3 deletions(-)\n>\n> diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh\n> index 1884177e293..1b775343adc 100644\n> --- a/t/test-lib-functions.sh\n> +++ b/t/test-lib-functions.sh\n> @@ -137,9 +137,44 @@ test_tick () {\n> # Stop execution and start a shell. This is useful for debugging tests.\n> #\n> # Be sure to remove all invocations of this command before submitting.\n> +#\n> +# Usage: test_pause [options]\n> +# -t\n> +# Use your original TERM instead of test-lib.sh's \"dumb\".\n> +# This usually restores color output in the invoked shell.\n> +# WARNING: this can break test reproducibility.\n> +# -s\n> +# Invoke $SHELL instead of $TEST_SHELL_PATH\n> +# WARNING: this can break test reproducibility.\n> +# -h\n> +# Use your original HOME instead of test-lib.sh's \"$TRASH_DIRECTORY\".\n> +# This allows you to use your regular shell environment and Git aliases.\n> +# WARNING: this can break test reproducibility.\n> +# CAUTION: this can overwrite files in your HOME.\n\nDo you want to add an option that implies the other three, so that\nfolks can do something like `test_pause -a` (or some other single\nletter) rather than `test_pause -t -s -h`?\n\n>\n> test_pause () {\n> - \"$TEST_SHELL_PATH\" <&6 >&5 2>&7\n> + PAUSE_TERM=$TERM &&\n> + PAUSE_SHELL=$TEST_SHELL_PATH &&\n> + PAUSE_HOME=$HOME &&\n> + while test $# != 0\n> + do\n> + case \"$1\" in\n> + -t)\n> + PAUSE_TERM=\"$USER_TERM\"\n> + ;;\n> + -s)\n> + PAUSE_SHELL=\"$SHELL\"\n> + ;;\n> + -h)\n> + PAUSE_HOME=\"$USER_HOME\"\n> + ;;\n> + *)\n> + break\n> + ;;\n> + esac\n> + shift\n> + done &&\n> + TERM=\"$PAUSE_TERM\" HOME=\"$PAUSE_HOME\" \"$PAUSE_SHELL\" <&6 >&5 2>&7\n> }\n>\n> # Wrap git with a debugger. Adding this to a command can make it easier\n> diff --git a/t/test-lib.sh b/t/test-lib.sh\n> index abcfbed6d61..132618991e2 100644\n> --- a/t/test-lib.sh\n> +++ b/t/test-lib.sh\n> @@ -585,8 +585,9 @@ else\n> }\n> fi\n>\n> +USER_TERM=\"$TERM\"\n> TERM=dumb\n> -export TERM\n> +export TERM USER_TERM\n>\n> error () {\n> say_color error \"error: $*\"\n> @@ -1380,9 +1381,10 @@ then\n> fi\n>\n> # Last-minute variable setup\n> +USER_HOME=\"$HOME\"\n> HOME=\"$TRASH_DIRECTORY\"\n> GNUPGHOME=\"$HOME/gnupg-home-not-used\"\n> -export HOME GNUPGHOME\n> +export HOME GNUPGHOME USER_HOME\n>\n> # Test repository\n> rm -fr \"$TRASH_DIRECTORY\" || {\n> --\n> gitgitgadget\n"},{"id":"434020","messageId":"46a845ee-f97b-6aed-1886-07b0ead63e58@gmail.com","threadId":"56325","inReplyTo":"CABPp-BFxDxD0Y5Ru+4VqebAJve4tjZk-4csN3JurC6hmRAUf4A@mail.gmail.com","subject":"Re: [PATCH v2 2/3] test-lib-functions: optionally keep HOME, TERM and SHELL in 'test_pause'","fromName":"Philippe Blain","fromEmail":"levraiphilippeblain@gmail.com","sentAt":"2021-08-28T14:50:28Z","receivedAt":"2021-08-28T14:52:37Z","isPatch":true,"sender":{"key":"levraiphilippeblain@gmail.com","avatar":"https://avatars.githubusercontent.com/u/44212482?v=4"},"body":"Hi Elijah,\n\nLe 2021-08-28 à 03:27, Elijah Newren a écrit :\n> On Fri, Aug 27, 2021 at 5:47 PM Philippe Blain via GitGitGadget\n> wrote:\n>> +#\n>> +# Usage: test_pause [options]\n>> +# -t\n>> +# Use your original TERM instead of test-lib.sh's \"dumb\".\n>> +# This usually restores color output in the invoked shell.\n>> +# WARNING: this can break test reproducibility.\n>> +# -s\n>> +# Invoke $SHELL instead of $TEST_SHELL_PATH\n>> +# WARNING: this can break test reproducibility.\n>> +# -h\n>> +# Use your original HOME instead of test-lib.sh's \"$TRASH_DIRECTORY\".\n>> +# This allows you to use your regular shell environment and Git aliases.\n>> +# WARNING: this can break test reproducibility.\n>> +# CAUTION: this can overwrite files in your HOME.\n> \n> Do you want to add an option that implies the other three, so that\n> folks can do something like `test_pause -a` (or some other single\n> letter) rather than `test_pause -t -s -h`?\n> \n>>\nYes, that would be even better. I'll add that for next version.\n\nThanks,\n\nPhilippe.\n"},{"id":"434443","messageId":"pull.1022.v3.git.1630503102.gitgitgadget@gmail.com","threadId":"56325","inReplyTo":"pull.1022.v2.git.1630111653.gitgitgadget@gmail.com","subject":"[PATCH v3 0/3] test-lib-functions.sh: keep user's HOME, TERM and SHELL for 'test_pause' and 'debug'","fromName":"Philippe Blain via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-09-01T13:31:39Z","receivedAt":"2021-09-01T13:31:48Z","isPatch":true,"sender":{"key":"levraiphilippeblain@gmail.com","avatar":"https://avatars.githubusercontent.com/u/44212482?v=4"},"body":"Changes since v2:\n\n * Added '-a' flag as suggested by Elijah, equivalent to '-t -s -h' for\n 'test_pause' and to '-t -h' for 'debug'\n\nv2:\n\n * added 1/3 as a preliminary step to use TEST_SHELL_PATH in test_pause\n instead of SHELL_PATH, as suggested by Carlo\n * implemented the change in behaviour through optional flags in both\n test_pause and debug. This seemed to be the simplest way to keep the\n current behaviour but also provide a way to improve the UX.\n\nv1: This series proposes two small quality-of-life improvements (in my\nopinion) to the 'test_pause' and 'debug' test functions: using the original\nvalues of HOME and TERM (before they are changed by the test framework) and\nusing SHELL instead of SHELL_PATH.\n\nThe later might be too big of a change, but I think it makes sense. We could\nadd a new GIT_TEST_* to conditionnaly change the behaviour, but I kept it\nsimple for v1.\n\nCheers, Philippe.\n\nPhilippe Blain (3):\n test-lib-functions: use 'TEST_SHELL_PATH' in 'test_pause'\n test-lib-functions: optionally keep HOME, TERM and SHELL in\n 'test_pause'\n test-lib-functions: optionally keep HOME and TERM in 'debug'\n\n t/test-lib-functions.sh | 104 ++++++++++++++++++++++++++++++++++------\n t/test-lib.sh | 6 ++-\n 2 files changed, 93 insertions(+), 17 deletions(-)\n\n\nbase-commit: 225bc32a989d7a22fa6addafd4ce7dcd04675dbf\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1022%2Fphil-blain%2Ftest-pause-and-debug-easier-v3\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1022/phil-blain/test-pause-and-debug-easier-v3\nPull-Request: https://github.com/gitgitgadget/git/pull/1022\n\nRange-diff vs v2:\n\n 1: 2f566f330e0 = 1: 2f566f330e0 test-lib-functions: use 'TEST_SHELL_PATH' in 'test_pause'\n 2: 00211457ece ! 2: 328b5d6e76f test-lib-functions: optionally keep HOME, TERM and SHELL in 'test_pause'\n @@ t/test-lib-functions.sh: test_tick () {\n +#\tThis allows you to use your regular shell environment and Git aliases.\n +#\tWARNING: this can break test reproducibility.\n +#\tCAUTION: this can overwrite files in your HOME.\n ++# -a\n ++#\tShortcut for -t -s -h\n \n test_pause () {\n -\t\"$TEST_SHELL_PATH\" <&6 >&5 2>&7\n @@ t/test-lib-functions.sh: test_tick () {\n +\t\t-h)\n +\t\t\tPAUSE_HOME=\"$USER_HOME\"\n +\t\t\t;;\n ++\t\t-a)\n ++\t\t\tPAUSE_TERM=\"$USER_TERM\"\n ++\t\t\tPAUSE_SHELL=\"$SHELL\"\n ++\t\t\tPAUSE_HOME=\"$USER_HOME\"\n ++\t\t\t;;\n +\t\t*)\n +\t\t\tbreak\n +\t\t\t;;\n 3: 1fac9baec1d ! 3: 4e43bd086b5 test-lib-functions: optionally keep HOME and TERM in 'debug'\n @@ t/test-lib-functions.sh: test_pause () {\n +#\tThis allows your debugger to find its config file in your home.\n +#\tWARNING: this can break test reproducibility.\n +#\tCAUTION: this can overwrite files in your HOME.\n ++# -a\n ++#\tShortcut for -t -h\n +#\n # Examples:\n # debug git checkout master\n @@ t/test-lib-functions.sh: test_pause () {\n +\t\t-h)\n +\t\t\tDEBUG_HOME=\"$USER_HOME\"\n +\t\t\t;;\n ++\t\t-a)\n ++\t\t\tDEBUG_TERM=\"$USER_TERM\"\n ++\t\t\tDEBUG_HOME=\"$USER_HOME\"\n ++\t\t\t;;\n +\t\t-d)\n +\t\t\tGIT_DEBUGGER=\"$2\" &&\n +\t\t\tshift\n\n-- \ngitgitgadget\n"},{"id":"434444","messageId":"328b5d6e76f598590d24b35ec23b5fd854c6cf05.1630503102.git.gitgitgadget@gmail.com","threadId":"56325","inReplyTo":"pull.1022.v3.git.1630503102.gitgitgadget@gmail.com","subject":"[PATCH v3 2/3] test-lib-functions: optionally keep HOME, TERM and SHELL in 'test_pause'","fromName":"Philippe Blain via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-09-01T13:31:41Z","receivedAt":"2021-09-01T13:31:49Z","isPatch":true,"sender":{"key":"levraiphilippeblain@gmail.com","avatar":"https://avatars.githubusercontent.com/u/44212482?v=4"},"body":"From: Philippe Blain \n\nThe 'test_pause' function, which is designed to help interactive\ndebugging and exploration of tests, currently inherits the value of HOME\nand TERM set by 'test-lib.sh': HOME=\"$TRASH_DIRECTORY\" and TERM=dumb. It\nalso invokes the shell defined by TEST_SHELL_PATH, which defaults to\n/bin/sh (through SHELL_PATH).\n\nChanging the value of HOME means that any customization configured in a\ndevelopers' shell startup files and any Git aliases defined in their\nglobal Git configuration file are not available in the shell invoked by\n'test_pause'.\n\nChanging the value of TERM to 'dumb' means that colored output\nis disabled for all commands in that shell.\n\nUsing /bin/sh as the shell invoked by 'test_pause' is not ideal since\nsome platforms (i.e. Debian and derivatives) use Dash as /bin/sh, and\nthis shell is usually compiled without readline support, which makes for\na poor interactive command line experience.\n\nTo make the interactive command line experience in the shell invoked by\n'test_pause' more pleasant, save the values of HOME and TERM in\nUSER_HOME and USER_TERM before changing them in test-lib.sh, and add\noptions to 'test_pause' to optionally use these variables to invoke the\nshell. Also add an option to invoke SHELL instead of TEST_SHELL_PATH, so\nthat developer's interactive shell is used.\n\nWe use options instead of changing the behaviour unconditionally since\nthese three variables can break test reproducibility. Moreover, using the\noriginal HOME means tests could overwrite files in a user's home\ndirectory. Be explicit about these caveats in the new 'Usage' section in\ntest-lib-functions.sh.\n\nSigned-off-by: Philippe Blain \n---\n t/test-lib-functions.sh | 44 ++++++++++++++++++++++++++++++++++++++++-\n t/test-lib.sh | 6 ++++--\n 2 files changed, 47 insertions(+), 3 deletions(-)\n\ndiff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh\nindex 1884177e293..4b667dc7e76 100644\n--- a/t/test-lib-functions.sh\n+++ b/t/test-lib-functions.sh\n@@ -137,9 +137,51 @@ test_tick () {\n # Stop execution and start a shell. This is useful for debugging tests.\n #\n # Be sure to remove all invocations of this command before submitting.\n+#\n+# Usage: test_pause [options]\n+# -t\n+#\tUse your original TERM instead of test-lib.sh's \"dumb\".\n+#\tThis usually restores color output in the invoked shell.\n+#\tWARNING: this can break test reproducibility.\n+# -s\n+#\tInvoke $SHELL instead of $TEST_SHELL_PATH\n+#\tWARNING: this can break test reproducibility.\n+# -h\n+#\tUse your original HOME instead of test-lib.sh's \"$TRASH_DIRECTORY\".\n+#\tThis allows you to use your regular shell environment and Git aliases.\n+#\tWARNING: this can break test reproducibility.\n+#\tCAUTION: this can overwrite files in your HOME.\n+# -a\n+#\tShortcut for -t -s -h\n \n test_pause () {\n-\t\"$TEST_SHELL_PATH\" <&6 >&5 2>&7\n+\tPAUSE_TERM=$TERM &&\n+\tPAUSE_SHELL=$TEST_SHELL_PATH &&\n+\tPAUSE_HOME=$HOME &&\n+\twhile test $# != 0\n+\tdo\n+\t\tcase \"$1\" in\n+\t\t-t)\n+\t\t\tPAUSE_TERM=\"$USER_TERM\"\n+\t\t\t;;\n+\t\t-s)\n+\t\t\tPAUSE_SHELL=\"$SHELL\"\n+\t\t\t;;\n+\t\t-h)\n+\t\t\tPAUSE_HOME=\"$USER_HOME\"\n+\t\t\t;;\n+\t\t-a)\n+\t\t\tPAUSE_TERM=\"$USER_TERM\"\n+\t\t\tPAUSE_SHELL=\"$SHELL\"\n+\t\t\tPAUSE_HOME=\"$USER_HOME\"\n+\t\t\t;;\n+\t\t*)\n+\t\t\tbreak\n+\t\t\t;;\n+\t\tesac\n+\t\tshift\n+\tdone &&\n+\tTERM=\"$PAUSE_TERM\" HOME=\"$PAUSE_HOME\" \"$PAUSE_SHELL\" <&6 >&5 2>&7\n }\n \n # Wrap git with a debugger. Adding this to a command can make it easier\ndiff --git a/t/test-lib.sh b/t/test-lib.sh\nindex abcfbed6d61..132618991e2 100644\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -585,8 +585,9 @@ else\n \t}\n fi\n \n+USER_TERM=\"$TERM\"\n TERM=dumb\n-export TERM\n+export TERM USER_TERM\n \n error () {\n \tsay_color error \"error: $*\"\n@@ -1380,9 +1381,10 @@ then\n fi\n \n # Last-minute variable setup\n+USER_HOME=\"$HOME\"\n HOME=\"$TRASH_DIRECTORY\"\n GNUPGHOME=\"$HOME/gnupg-home-not-used\"\n-export HOME GNUPGHOME\n+export HOME GNUPGHOME USER_HOME\n \n # Test repository\n rm -fr \"$TRASH_DIRECTORY\" || {\n-- \ngitgitgadget\n\n"},{"id":"434445","messageId":"2f566f330e01ed4249edad54671e98301dbeec5a.1630503102.git.gitgitgadget@gmail.com","threadId":"56325","inReplyTo":"pull.1022.v3.git.1630503102.gitgitgadget@gmail.com","subject":"[PATCH v3 1/3] test-lib-functions: use 'TEST_SHELL_PATH' in 'test_pause'","fromName":"Philippe Blain via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-09-01T13:31:40Z","receivedAt":"2021-09-01T13:31:51Z","isPatch":true,"sender":{"key":"levraiphilippeblain@gmail.com","avatar":"https://avatars.githubusercontent.com/u/44212482?v=4"},"body":"From: Philippe Blain \n\n3f824e91c8 (t/Makefile: introduce TEST_SHELL_PATH, 2017-12-08)\nmade it easy to use a different shell for the tests than 'SHELL_PATH'\nused at compile time. But 'test_pause' still invokes 'SHELL_PATH'.\n\nIf TEST_SHELL_PATH is set, invoke that shell in 'test_pause' for\nconsistency.\n\nSuggested-by: Carlo Marcelo Arenas Belón \nSigned-off-by: Philippe Blain \n---\n t/test-lib-functions.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh\nindex e28411bb75a..1884177e293 100644\n--- a/t/test-lib-functions.sh\n+++ b/t/test-lib-functions.sh\n@@ -139,7 +139,7 @@ test_tick () {\n # Be sure to remove all invocations of this command before submitting.\n \n test_pause () {\n-\t\"$SHELL_PATH\" <&6 >&5 2>&7\n+\t\"$TEST_SHELL_PATH\" <&6 >&5 2>&7\n }\n \n # Wrap git with a debugger. Adding this to a command can make it easier\n-- \ngitgitgadget\n\n"},{"id":"434446","messageId":"4e43bd086b5d7768afd2c35b1bab19120fb87ead.1630503102.git.gitgitgadget@gmail.com","threadId":"56325","inReplyTo":"pull.1022.v3.git.1630503102.gitgitgadget@gmail.com","subject":"[PATCH v3 3/3] test-lib-functions: optionally keep HOME and TERM in 'debug'","fromName":"Philippe Blain via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-09-01T13:31:42Z","receivedAt":"2021-09-01T13:31:52Z","isPatch":true,"sender":{"key":"levraiphilippeblain@gmail.com","avatar":"https://avatars.githubusercontent.com/u/44212482?v=4"},"body":"From: Philippe Blain \n\nThe 'debug' function in test-lib-functions.sh is used to invoke a\ndebugger at a specific line in a test. It inherits the value of HOME and\nTERM set by 'test-lib.sh': HOME=\"$TRASH_DIRECTORY\" and TERM=dumb.\n\nChanging the value of HOME means that any customization configured in a\ndevelopers' debugger configuration file (like $HOME/.gdbinit or\n$HOME/.lldbinit) are not available in the debugger invoked by\n'test_pause'.\n\nChanging the value of TERM to 'dumb' means that colored output\nis disabled in the debugger.\n\nTo make the debugging experience with 'debug' more pleasant, leverage\nthe variables USER_HOME and USER_TERM, added in the previous commit, to\noptionally set HOME and TERM before invoking the debugger.\n\nAdd the same warnings as for 'test_pause' in the \"Usage\" section.\n\nSigned-off-by: Philippe Blain \n---\n t/test-lib-functions.sh | 60 +++++++++++++++++++++++++++++++----------\n 1 file changed, 46 insertions(+), 14 deletions(-)\n\ndiff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh\nindex 4b667dc7e76..537fd88e686 100644\n--- a/t/test-lib-functions.sh\n+++ b/t/test-lib-functions.sh\n@@ -187,25 +187,57 @@ test_pause () {\n # Wrap git with a debugger. Adding this to a command can make it easier\n # to understand what is going on in a failing test.\n #\n+# Usage: debug [options] [git command]\n+# -d \n+# --debugger=\n+#\tUse instead of GDB\n+# -t\n+#\tUse your original TERM instead of test-lib.sh's \"dumb\".\n+#\tThis usually restores color output in the debugger.\n+#\tWARNING: this can break test reproducibility.\n+# -h\n+#\tUse your original HOME instead of test-lib.sh's \"$TRASH_DIRECTORY\".\n+#\tThis allows your debugger to find its config file in your home.\n+#\tWARNING: this can break test reproducibility.\n+#\tCAUTION: this can overwrite files in your HOME.\n+# -a\n+#\tShortcut for -t -h\n+#\n # Examples:\n # debug git checkout master\n # debug --debugger=nemiver git $ARGS\n # debug -d \"valgrind --tool=memcheck --track-origins=yes\" git $ARGS\n debug () {\n-\tcase \"$1\" in\n-\t-d)\n-\t\tGIT_DEBUGGER=\"$2\" &&\n-\t\tshift 2\n-\t\t;;\n-\t--debugger=*)\n-\t\tGIT_DEBUGGER=\"${1#*=}\" &&\n-\t\tshift 1\n-\t\t;;\n-\t*)\n-\t\tGIT_DEBUGGER=1\n-\t\t;;\n-\tesac &&\n-\tGIT_DEBUGGER=\"${GIT_DEBUGGER}\" \"$@\" <&6 >&5 2>&7\n+\tGIT_DEBUGGER=1 &&\n+\tDEBUG_TERM=$TERM &&\n+\tDEBUG_HOME=$HOME &&\n+\twhile test $# != 0\n+\tdo\n+\t\tcase \"$1\" in\n+\t\t-t)\n+\t\t\tDEBUG_TERM=\"$USER_TERM\"\n+\t\t\t;;\n+\t\t-h)\n+\t\t\tDEBUG_HOME=\"$USER_HOME\"\n+\t\t\t;;\n+\t\t-a)\n+\t\t\tDEBUG_TERM=\"$USER_TERM\"\n+\t\t\tDEBUG_HOME=\"$USER_HOME\"\n+\t\t\t;;\n+\t\t-d)\n+\t\t\tGIT_DEBUGGER=\"$2\" &&\n+\t\t\tshift\n+\t\t\t;;\n+\t\t--debugger=*)\n+\t\t\tGIT_DEBUGGER=\"${1#*=}\"\n+\t\t\t;;\n+\t\t*)\n+\t\t\tbreak\n+\t\t\t;;\n+\t\tesac\n+\t\tshift\n+\tdone &&\n+\tTERM=\"$DEBUG_TERM\" HOME=\"$DEBUG_HOME\" GIT_DEBUGGER=\"${GIT_DEBUGGER}\" \"$@\" <&6 >&5 2>&7\n }\n \n # Usage: test_commit [options] [ [ []]]\n-- \ngitgitgadget\n"},{"id":"434467","messageId":"xmqqsfyooydz.fsf@gitster.g","threadId":"56325","inReplyTo":"2f566f330e01ed4249edad54671e98301dbeec5a.1630503102.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v3 1/3] test-lib-functions: use 'TEST_SHELL_PATH' in 'test_pause'","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-09-01T20:04:56Z","receivedAt":"2021-09-01T20:05:02Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Philippe Blain via GitGitGadget\" writes:\n\n> From: Philippe Blain \n>\n> 3f824e91c8 (t/Makefile: introduce TEST_SHELL_PATH, 2017-12-08)\n> made it easy to use a different shell for the tests than 'SHELL_PATH'\n> used at compile time. But 'test_pause' still invokes 'SHELL_PATH'.\n>\n> If TEST_SHELL_PATH is set, invoke that shell in 'test_pause' for\n> consistency.\n>\n> Suggested-by: Carlo Marcelo Arenas Belón \n> Signed-off-by: Philippe Blain \n> ---\n> t/test-lib-functions.sh | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh\n> index e28411bb75a..1884177e293 100644\n> --- a/t/test-lib-functions.sh\n> +++ b/t/test-lib-functions.sh\n> @@ -139,7 +139,7 @@ test_tick () {\n> # Be sure to remove all invocations of this command before submitting.\n> \n> test_pause () {\n> -\t\"$SHELL_PATH\" <&6 >&5 2>&7\n> +\t\"$TEST_SHELL_PATH\" <&6 >&5 2>&7\n> }\n> \n> # Wrap git with a debugger. Adding this to a command can make it easier\n\nSensible.\n"},{"id":"434468","messageId":"xmqq7dg0oxdm.fsf@gitster.g","threadId":"56325","inReplyTo":"328b5d6e76f598590d24b35ec23b5fd854c6cf05.1630503102.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v3 2/3] test-lib-functions: optionally keep HOME, TERM and SHELL in 'test_pause'","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-09-01T20:26:45Z","receivedAt":"2021-09-01T20:26:49Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Philippe Blain via GitGitGadget\" writes:\n\n> +# Usage: test_pause [options]\n> +# -t\n> +#\tUse your original TERM instead of test-lib.sh's \"dumb\".\n> +#\tThis usually restores color output in the invoked shell.\n> +#\tWARNING: this can break test reproducibility.\n> +# -s\n> +#\tInvoke $SHELL instead of $TEST_SHELL_PATH\n> +#\tWARNING: this can break test reproducibility.\n> +# -h\n> +#\tUse your original HOME instead of test-lib.sh's \"$TRASH_DIRECTORY\".\n> +#\tThis allows you to use your regular shell environment and Git aliases.\n> +#\tWARNING: this can break test reproducibility.\n> +#\tCAUTION: this can overwrite files in your HOME.\n> +# -a\n> +#\tShortcut for -t -s -h\n\nAs this is not end-user facing but facing our developer, I do not\ndeeply care, but I find the warnings in this help text problematic.\n\nBecause a new process instance of $PAUSE_SHELL is run, the options\nyou add when inserting test_pause does not affect what happens in\nthe tests after you exit the $PAUSE_SHELL [*], right?\n\nOf course, you can modify the repository or the working tree files\nused in the test in the $PAUSE_SHELL, and that can break \"test\nreproducibility\"---if you run \"git ls-files -s\" and take its output\nin a temporary file, a step later in the test that runs \"git status\"\nwill see an extra untracked file, for example, and such a change may\n(or may not) unnecessarily break the tests.\n\nBut it is not anything new introduced by these options. It is\ninherent to test_pause itself.\n\nIf we want to add a warning to the help text here, I think it should\nbe written in such a way that it is clear that the warning applies\nto any use of the test_pause helper, not just to the form with the\noptions.\n\nThanks.\n\n\n[Footnote]\n\n* If we had an alternative implementation of test_pause that does\n not use a separate $PAUSE_SHELL process, but instead like\n inserting a read-eval-print loop, that would be far more powerful\n and useful debugging aid. You can not just stop the execution of\n the test, and observe the files in the test repository and the\n environment variables---you can also access shell variables and\n functions.\n\n Such a test_pause from another world would deserve a \"if you touch\n anything, the damage is permanent\" warning even more than the\n current one, because you can modify even a shell variable to\n affect the execution of the test after you leave the paused state.\n"},{"id":"434476","messageId":"CABPp-BFEvSXHAzFFs8VaWK5QUTj6Zyow9863p=qM6G8v3OPy1g@mail.gmail.com","threadId":"56325","inReplyTo":"xmqq7dg0oxdm.fsf@gitster.g","subject":"Re: [PATCH v3 2/3] test-lib-functions: optionally keep HOME, TERM and SHELL in 'test_pause'","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2021-09-01T21:52:38Z","receivedAt":"2021-09-01T21:52:51Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Wed, Sep 1, 2021 at 1:26 PM Junio C Hamano wrote:\n>\n> \"Philippe Blain via GitGitGadget\" writes:\n>\n> > +# Usage: test_pause [options]\n> > +# -t\n> > +# Use your original TERM instead of test-lib.sh's \"dumb\".\n> > +# This usually restores color output in the invoked shell.\n> > +# WARNING: this can break test reproducibility.\n> > +# -s\n> > +# Invoke $SHELL instead of $TEST_SHELL_PATH\n> > +# WARNING: this can break test reproducibility.\n> > +# -h\n> > +# Use your original HOME instead of test-lib.sh's \"$TRASH_DIRECTORY\".\n> > +# This allows you to use your regular shell environment and Git aliases.\n> > +# WARNING: this can break test reproducibility.\n> > +# CAUTION: this can overwrite files in your HOME.\n> > +# -a\n> > +# Shortcut for -t -s -h\n>\n> As this is not end-user facing but facing our developer, I do not\n> deeply care, but I find the warnings in this help text problematic.\n>\n> Because a new process instance of $PAUSE_SHELL is run, the options\n> you add when inserting test_pause does not affect what happens in\n> the tests after you exit the $PAUSE_SHELL [*], right?\n\nI think the warning was less about what happens after test_pause is\ncomplete and the testcase resumes, and more intended to convey that if\nthe user tries to manually copy & paste snippets of code from the test\ninto $PAUSE_SHELL, then the use of these flags can cause the result of\nthose pasted commands to differ. If so, a small rewording might be in\norder, e.g. \"WARNING: this can cause commands run in the paused shell\nto give different results\". I'd also say the CAUTION would perhaps\nbenefit from some rewording (since the option itself doesn't cause\nfiles to be overwritten), e.g. \"CAUTION: using this option and copying\ncommands from the test script into the paused shell might result in\nfiles in your HOME being overwritten\".\n\n> Of course, you can modify the repository or the working tree files\n> used in the test in the $PAUSE_SHELL, and that can break \"test\n> reproducibility\"---if you run \"git ls-files -s\" and take its output\n> in a temporary file, a step later in the test that runs \"git status\"\n> will see an extra untracked file, for example, and such a change may\n> (or may not) unnecessarily break the tests.\n>\n> But it is not anything new introduced by these options. It is\n> inherent to test_pause itself.\n>\n> If we want to add a warning to the help text here, I think it should\n> be written in such a way that it is clear that the warning applies\n> to any use of the test_pause helper, not just to the form with the\n> options.\n>\n> Thanks.\n>\n>\n> [Footnote]\n>\n> * If we had an alternative implementation of test_pause that does\n> not use a separate $PAUSE_SHELL process, but instead like\n> inserting a read-eval-print loop, that would be far more powerful\n> and useful debugging aid. You can not just stop the execution of\n> the test, and observe the files in the test repository and the\n> environment variables---you can also access shell variables and\n> functions.\n>\n> Such a test_pause from another world would deserve a \"if you touch\n> anything, the damage is permanent\" warning even more than the\n> current one, because you can modify even a shell variable to\n> affect the execution of the test after you leave the paused state.\n"},{"id":"434479","messageId":"xmqqa6kvoptx.fsf@gitster.g","threadId":"56325","inReplyTo":"CABPp-BFEvSXHAzFFs8VaWK5QUTj6Zyow9863p=qM6G8v3OPy1g@mail.gmail.com","subject":"Re: [PATCH v3 2/3] test-lib-functions: optionally keep HOME, TERM and SHELL in 'test_pause'","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-09-01T23:09:46Z","receivedAt":"2021-09-01T23:09:53Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Elijah Newren writes:\n\n>> Because a new process instance of $PAUSE_SHELL is run, the options\n>> you add when inserting test_pause does not affect what happens in\n>> the tests after you exit the $PAUSE_SHELL [*], right?\n>\n> I think the warning was less about what happens after test_pause is\n> complete and the testcase resumes, and more intended to convey that if\n> the user tries to manually copy & paste snippets of code from the test\n> into $PAUSE_SHELL, then the use of these flags can cause the result of\n> those pasted commands to differ.\n\nBut the difference of TERM and SHELL are much smaller issue when one\ncuts and pastes the lines from the test script. Even if you do not\nuse -s or -t options, shell variables and helpers won't be available\nto the interactive session given by test_pause, and that is a much\nbigger difference between the \"real\" shell environment that is\nrunning the tests and the user's shell environment test_pause gives\nus. That is why I think the WARNING should be attached _directly_\nto the description of test_pause and not to individual \"this uses\ndifferent X environment compared to the real shell environment\nrunning the tests\" options.\n\nThe updated $HOME does deserve a special mention, as .gitconfig from\nit would further affect the tests and hide the bug you are hunting\nfor, and the CAUTION is a good thing to have, even if you do not\nplan to modify anything in the test_pause sight-seeing session.\n"},{"id":"434523","messageId":"3256e230-9db4-57fa-8b43-b9d7e18ba725@gmail.com","threadId":"56325","inReplyTo":"xmqqa6kvoptx.fsf@gitster.g","subject":"Re: [PATCH v3 2/3] test-lib-functions: optionally keep HOME, TERM and SHELL in 'test_pause'","fromName":"Philippe Blain","fromEmail":"levraiphilippeblain@gmail.com","sentAt":"2021-09-02T13:10:51Z","receivedAt":"2021-09-02T13:10:46Z","isPatch":true,"sender":{"key":"levraiphilippeblain@gmail.com","avatar":"https://avatars.githubusercontent.com/u/44212482?v=4"},"body":"Hi Elijah and Junio,\n\nLe 2021-09-01 à 19:09, Junio C Hamano a écrit :\n> Elijah Newren writes:\n> \n>>> Because a new process instance of $PAUSE_SHELL is run, the options\n>>> you add when inserting test_pause does not affect what happens in\n>>> the tests after you exit the $PAUSE_SHELL [*], right?\n>>\n>> I think the warning was less about what happens after test_pause is\n>> complete and the testcase resumes, and more intended to convey that if\n>> the user tries to manually copy & paste snippets of code from the test\n>> into $PAUSE_SHELL, then the use of these flags can cause the result of\n>> those pasted commands to differ.\n\nYes, I added the warnings based on the reticence I got in v1 to change the\nbehaviour without adding flags.\n\n> \n> But the difference of TERM and SHELL are much smaller issue when one\n> cuts and pastes the lines from the test script. Even if you do not\n> use -s or -t options, shell variables and helpers won't be available\n> to the interactive session given by test_pause, and that is a much\n> bigger difference between the \"real\" shell environment that is\n> running the tests and the user's shell environment test_pause gives\n> us. That is why I think the WARNING should be attached _directly_\n> to the description of test_pause and not to individual \"this uses\n> different X environment compared to the real shell environment\n> running the tests\" options.\n\nOK, I can make that tweak.\n\n\nLe 2021-09-01 à 17:52, Elijah Newren a écrit :\n-- snip --\n> those pasted commands to differ. If so, a small rewording might be in\n> order, e.g. \"WARNING: this can cause commands run in the paused shell\n> to give different results\". I'd also say the CAUTION would perhaps\n> benefit from some rewording (since the option itself doesn't cause\n> files to be overwritten), e.g. \"CAUTION: using this option and copying\n> commands from the test script into the paused shell might result in\n> files in your HOME being overwritten\".\n\nThanks for these suggestions, that wording does seem clearer.\n\nPhilippe.\n"},{"id":"434754","messageId":"pull.1022.v4.git.1630902006.gitgitgadget@gmail.com","threadId":"56325","inReplyTo":"pull.1022.v3.git.1630503102.gitgitgadget@gmail.com","subject":"[PATCH v4 0/3] test-lib-functions.sh: keep user's HOME, TERM and SHELL for 'test_pause' and 'debug'","fromName":"Philippe Blain via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-09-06T04:20:03Z","receivedAt":"2021-09-06T04:20:11Z","isPatch":true,"sender":{"key":"levraiphilippeblain@gmail.com","avatar":"https://avatars.githubusercontent.com/u/44212482?v=4"},"body":"Changes since v3:\n\n * 2/3: improved the wording for the warning and caution as suggested by\n Elijah,, and moved the warning so it relates to the use of test_pause\n itself, not just the new flags, as suggested by Junio. Adapted the commit\n messages accordingly.\n * 3/3: changed the approach: instead of changing HOME, just copy ~/.gdbinit\n and ~/.lldbinit to the test HOME, as suggested by Carlo. This seems safer\n as this way $USER_HOME/.gitconfig does not interfere with the behaviour\n of the command being debugged (as Junio remarked in [1], but for\n test_pause). If other config files are needed for other debuggers, they\n can be added when the need arises.\n * [23]/3: also adapted the synopsys of 'test_pause' and 'debug' in t/README\n for better discoverability of the new features.\n\n[1] https://lore.kernel.org/git/xmqqa6kvoptx.fsf@gitster.g/\n\nv3:\n\n * Added '-a' flag as suggested by Elijah, equivalent to '-t -s -h' for\n 'test_pause' and to '-t -h' for 'debug'\n\nv2:\n\n * added 1/3 as a preliminary step to use TEST_SHELL_PATH in test_pause\n instead of SHELL_PATH, as suggested by Carlo\n * implemented the change in behaviour through optional flags in both\n test_pause and debug. This seemed to be the simplest way to keep the\n current behaviour but also provide a way to improve the UX.\n\nv1: This series proposes two small quality-of-life improvements (in my\nopinion) to the 'test_pause' and 'debug' test functions: using the original\nvalues of HOME and TERM (before they are changed by the test framework) and\nusing SHELL instead of SHELL_PATH.\n\nThe later might be too big of a change, but I think it makes sense. We could\nadd a new GIT_TEST_* to conditionnaly change the behaviour, but I kept it\nsimple for v1.\n\nCheers, Philippe.\n\nPhilippe Blain (3):\n test-lib-functions: use 'TEST_SHELL_PATH' in 'test_pause'\n test-lib-functions: optionally keep HOME, TERM and SHELL in\n 'test_pause'\n test-lib-functions: keep user's debugger config files and TERM in\n 'debug'\n\n t/README | 11 ++--\n t/test-lib-functions.sh | 113 ++++++++++++++++++++++++++++++++++------\n t/test-lib.sh | 6 ++-\n 3 files changed, 109 insertions(+), 21 deletions(-)\n\n\nbase-commit: 225bc32a989d7a22fa6addafd4ce7dcd04675dbf\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1022%2Fphil-blain%2Ftest-pause-and-debug-easier-v4\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1022/phil-blain/test-pause-and-debug-easier-v4\nPull-Request: https://github.com/gitgitgadget/git/pull/1022\n\nRange-diff vs v3:\n\n 1: 2f566f330e0 = 1: 2f566f330e0 test-lib-functions: use 'TEST_SHELL_PATH' in 'test_pause'\n 2: 328b5d6e76f ! 2: a231d560e68 test-lib-functions: optionally keep HOME, TERM and SHELL in 'test_pause'\n @@ Commit message\n that developer's interactive shell is used.\n \n We use options instead of changing the behaviour unconditionally since\n - these three variables can break test reproducibility. Moreover, using the\n - original HOME means tests could overwrite files in a user's home\n - directory. Be explicit about these caveats in the new 'Usage' section in\n - test-lib-functions.sh.\n + these three variables can slightly change command behaviour. Moreover,\n + using the original HOME means commands could overwrite files in a user's\n + home directory. Be explicit about these caveats in the new 'Usage'\n + section in test-lib-functions.sh.\n \n + Finally, add '[options]' to the test_pause synopsys in t/README, and\n + mention that the full list of helper functions and their options can be\n + found in test-lib-functions.sh.\n +\n + Helped-by: Elijah Newren \n Signed-off-by: Philippe Blain \n \n + ## t/README ##\n +@@ t/README: Test harness library\n + --------------------\n + \n + There are a handful helper functions defined in the test harness\n +-library for your script to use.\n ++library for your script to use. Some of them are listed below;\n ++see test-lib-functions.sh for the full list and their options.\n + \n + - test_expect_success []