| Message ID | pull.1022.git.1629393395.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | test-lib-functions.sh: keep user's HOME, TERM and SHELL for 'test_pause' and 'debug' | expand |
On Thu, Aug 19, 2021 at 1:16 PM Philippe Blain via GitGitGadget <gitgitgadget@gmail.com> wrote: > This series proposes two small quality-of-life improvements (in my opinion) > to the 'test_pause' and 'debug' test functions: using the original values of > HOME and TERM (before they are changed by the test framework) and using > SHELL instead of SHELL_PATH. > > The later might be too big of a change, but I think it makes sense. We could > add a new GIT_TEST_* to conditionnaly change the behaviour, but I kept it > simple for v1. I also find the test_pause() user-experience suboptimal and appreciate the idea of improving it. However, this approach seems fatally flawed. In particular, setting HOME to the user's real home directory could lead to undesirable results. When I'm using test_pause() to debug a problem with a test, I'm not just inspecting the test state, but I quite often interact with the state using the same Git commands as the test itself would use. Hence, it is very common for me to pause the test just before the suspect commands and then run those commands manually (instead of allowing the test script to do so). In such a scenario, HOME must be pointing at the test's home directory, not at my real home directory. Perhaps there's some way to achieve your goal for at least certain shells by detecting the shell and taking advantage of special shell features which allow you to launch the shell and run some canned commands to set up the shell as desired before dropping into an interactive session[1] -- but it's just an idle thought. [1]: For Bash, for instance, a quick bit of Googling leads to: https://serverfault.com/a/586272
Eric Sunshine <sunshine@sunshineco.com> writes: > I also find the test_pause() user-experience suboptimal and appreciate > the idea of improving it. However, this approach seems fatally flawed. > In particular, setting HOME to the user's real home directory could > lead to undesirable results. When I'm using test_pause() to debug a > problem with a test, I'm not just inspecting the test state, but I > quite often interact with the state using the same Git commands as the > test itself would use. Yes, I do agree with you that it is a valid concern. I wonder if the developers can configure tools used during debugging session with XDG so that HOME can be kept for the "fake user that ran the test suite, with the fake user's configuration"?
On Thu, Aug 19, 2021 at 11:10 AM Eric Sunshine <sunshine@sunshineco.com> wrote: > > On Thu, Aug 19, 2021 at 1:16 PM Philippe Blain via GitGitGadget > <gitgitgadget@gmail.com> wrote: > > This series proposes two small quality-of-life improvements (in my opinion) > > to the 'test_pause' and 'debug' test functions: using the original values of > > HOME and TERM (before they are changed by the test framework) and using > > SHELL instead of SHELL_PATH. > > > > The later might be too big of a change, but I think it makes sense. We could > > add a new GIT_TEST_* to conditionnaly change the behaviour, but I kept it > > simple for v1. > > I also find the test_pause() user-experience suboptimal and appreciate > the idea of improving it. However, this approach seems fatally flawed. > In particular, setting HOME to the user's real home directory could > lead to undesirable results. When I'm using test_pause() to debug a > problem with a test, I'm not just inspecting the test state, but I > quite often interact with the state using the same Git commands as the > test itself would use. Hence, it is very common for me to pause the > test just before the suspect commands and then run those commands > manually (instead of allowing the test script to do so). In such a > scenario, HOME must be pointing at the test's home directory, not at > my real home directory. I agree, but I worry that it's not just HOME. I'd think the point of test_pause is to let you interact with the repository state while getting the same results that the test framework would, and I think some tests could be affected by TERM and SHELL too (e.g. perhaps the recent issues with COLUMNS). Granted, I suspect far fewer tests would be affected by those, but I'm not sure I like the idea of inability to reproduce the same issues. Maybe we just need a different construct or special flags to test_pause? > Perhaps there's some way to achieve your goal for at least certain > shells by detecting the shell and taking advantage of special shell > features which allow you to launch the shell and run some canned > commands to set up the shell as desired before dropping into an > interactive session[1] -- but it's just an idle thought. > > [1]: For Bash, for instance, a quick bit of Googling leads to: > https://serverfault.com/a/586272
On Thu, Aug 19, 2021 at 4:03 PM Elijah Newren <newren@gmail.com> wrote: > On Thu, Aug 19, 2021 at 11:10 AM Eric Sunshine <sunshine@sunshineco.com> wrote: > > I also find the test_pause() user-experience suboptimal and appreciate > > the idea of improving it. However, this approach seems fatally flawed. > > In particular, setting HOME to the user's real home directory could > > lead to undesirable results. When I'm using test_pause() to debug a > > problem with a test, I'm not just inspecting the test state, but I > > quite often interact with the state using the same Git commands as the > > test itself would use. Hence, it is very common for me to pause the > > test just before the suspect commands and then run those commands > > manually (instead of allowing the test script to do so). In such a > > scenario, HOME must be pointing at the test's home directory, not at > > my real home directory. > > I agree, but I worry that it's not just HOME. I'd think the point of > test_pause is to let you interact with the repository state while > getting the same results that the test framework would, and I think > some tests could be affected by TERM and SHELL too (e.g. perhaps the > recent issues with COLUMNS). Granted, I suspect far fewer tests would > be affected by those, but I'm not sure I like the idea of inability to > reproduce the same issues. Oh, indeed. I didn't mean to imply that HOME is the only problematic one; they all are since, as you say, they can impact correctness and reproducibility of the tests themselves. I called out HOME specially because of the potential danger involved with pointing it at the user's real home directory since it could very well lead to clobbering of precious files and other settings belonging to the user.
On Thu, Aug 19, 2021 at 3:58 PM Junio C Hamano <gitster@pobox.com> wrote: > Eric Sunshine <sunshine@sunshineco.com> writes: > > I also find the test_pause() user-experience suboptimal and appreciate > > the idea of improving it. However, this approach seems fatally flawed. > > In particular, setting HOME to the user's real home directory could > > lead to undesirable results. When I'm using test_pause() to debug a > > problem with a test, I'm not just inspecting the test state, but I > > quite often interact with the state using the same Git commands as the > > test itself would use. > > Yes, I do agree with you that it is a valid concern. > > I wonder if the developers can configure tools used during debugging > session with XDG so that HOME can be kept for the "fake user that > ran the test suite, with the fake user's configuration"? I haven't studied XDG deeply enough to answer, though it would not help macOS or Windows users (but that's not an argument against pursuing an XDG solution -- it might still help some developers).
Hi everyone, Le 2021-08-19 à 16:11, Eric Sunshine a écrit : > On Thu, Aug 19, 2021 at 4:03 PM Elijah Newren <newren@gmail.com> wrote: >> On Thu, Aug 19, 2021 at 11:10 AM Eric Sunshine <sunshine@sunshineco.com> wrote: >>> In such a >>> scenario, HOME must be pointing at the test's home directory, not at >>> my real home directory. >> >> I agree, but I worry that it's not just HOME. I'd think the point of >> test_pause is to let you interact with the repository state while >> getting the same results that the test framework would, and I think >> some tests could be affected by TERM and SHELL too (e.g. perhaps the >> recent issues with COLUMNS). Granted, I suspect far fewer tests would >> be affected by those, but I'm not sure I like the idea of inability to >> reproduce the same issues. > > Oh, indeed. I didn't mean to imply that HOME is the only problematic > one; they all are since, as you say, they can impact correctness and > reproducibility of the tests themselves. I called out HOME specially > because of the potential danger involved with pointing it at the > user's real home directory since it could very well lead to clobbering > of precious files and other settings belonging to the user. > Thanks everyone for sharing their input and concerns. I understand that the behaviour change might not be wanted all the time, or by everyone. I also did not think about the implications of changing $HOME that could lead to the test framework overwriting stuff in my home. I checked the tests and there are only a handful of them that seem to reference HOME, but still, for those tests it would be undesirable to reset HOME. In light of this I'm thinking of simply adding flags to 'test_pause' and 'debug' to signal that one wants to use their original TERM, HOME and SHELL, with appropriate caveats in the description of the functions: test_pause # original behaviour test_pause -t # use USER_TERM test_pause -s # use SHELL instead of TEST_SHELL_PATH test_pause -h # use USER_HOME and combinations of these three. For 'debug', Carlo's idea of just symlinking/copying gdbinit and/or llldbinit to the test HOME might be easier, and would cover the majority of developers, I think. As for TERM, we could do 'debug -t' as above, or use USER_TERM always... I'll explore these ideas before sending v2. Thanks, Philippe.
On Fri, Aug 20, 2021 at 8:12 AM Philippe Blain <levraiphilippeblain@gmail.com> wrote: > Le 2021-08-19 à 16:11, Eric Sunshine a écrit : > > Oh, indeed. I didn't mean to imply that HOME is the only problematic > > one; they all are since, as you say, they can impact correctness and > > reproducibility of the tests themselves. I called out HOME specially > > because of the potential danger involved with pointing it at the > > user's real home directory since it could very well lead to clobbering > > of precious files and other settings belonging to the user. > > I also did not think about the implications of changing $HOME that could lead to the > test framework overwriting stuff in my home. I checked the tests and there are only > a handful of them that seem to reference HOME, but still, for those tests it would be > undesirable to reset HOME. It's not just tests which reference HOME explicitly which are problematic. Git commands themselves access files and configuration pointed at by HOME. Worse, Git commands invoked by tests can alter configuration, so the potential for damage is wider than the few tests which reference HOME explicitly. > In light of this I'm thinking of simply adding flags to 'test_pause' and 'debug' to signal > that one wants to use their original TERM, HOME and SHELL, with appropriate caveats in > the description of the functions: > > test_pause # original behaviour > test_pause -t # use USER_TERM > test_pause -s # use SHELL instead of TEST_SHELL_PATH > test_pause -h # use USER_HOME A (very) stray thought I had: Rather than mucking with the environment variables -- which can impact test reproducibility and correctness and can potentially damage the user's precious files and configuration -- another possibility might be to detect which shell is being used (whether it be bash, zsh, etc.), and then enable certain useful options specific to the shell, such as tab-completion, colors, etc. This approach doesn't give developers the customized shell experience they're used to (it wouldn't have their aliases or configuration, for instance), but it at least might make the test_pause() experience a bit more pleasant.
On Fri, Aug 20, 2021 at 08:12:50AM -0400, Philippe Blain wrote: > Thanks everyone for sharing their input and concerns. I understand that the behaviour > change might not be wanted all the time, or by everyone. > > I also did not think about the implications of changing $HOME that could lead to the > test framework overwriting stuff in my home. I checked the tests and there are only > a handful of them that seem to reference HOME, but still, for those tests it would be > undesirable to reset HOME. > > In light of this I'm thinking of simply adding flags to 'test_pause' and 'debug' to signal > that one wants to use their original TERM, HOME and SHELL, with appropriate caveats in > the description of the functions: > > test_pause # original behaviour > test_pause -t # use USER_TERM > test_pause -s # use SHELL instead of TEST_SHELL_PATH > test_pause -h # use USER_HOME > > and combinations of these three. > > For 'debug', Carlo's idea of just symlinking/copying gdbinit and/or llldbinit to the test > HOME might be easier, and would cover the majority of developers, I think. As for TERM, > we could do 'debug -t' as above, or use USER_TERM always... > > I'll explore these ideas before sending v2. As an even more different alternative, what do you think about making it easier to break out of the test suite at a particular state? That would let you inspect it from your regular shell environment. E.g., most of my test debugging happens via: ./t1234-foo.sh -i cd trash\ directory-1234-foo/ gdb --args ../../git some-command That implies that the test you're interested in is actually failing (though I have been known to insert "false &&" to make it so), and that running the test doesn't wreck the on-disk state. But what if we could do something like: ./t1234-foo.sh --debug=42 to stop _before_ running test 42, print out the commands it would run, and leave the trash directory so that you can inspect it yourself? That does not have the "resume after I'm done inspecting" property that test_pause, but IMHO that is not really that useful for debugging. It will also occasionally cause headaches when a test relies on other parts of the environment (e.g., environment variables defined previously, or functions defined in the script). But I find those are usually a minority of cases. -Peff