| Message ID | 20201017075455.9660-5-charvi077@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | modernizing the test scripts | expand |
Hi Charvi Congratulations on posting your first patch series. On 17/10/2020 08:54, Charvi Mendiratta wrote: > Avoid using `cd` outside of subshells since, if the test fails, there is no guarantee that the current working directory is the expected one, which may cause subsequent tests to run in the wrong directory. That is an accurate description of why we want to avoid using `cd` outside of subshells. However this conversion is converting `cd` inside a subshell to use `git -C`. I think that is worthwhile as it avoids having to use a subshell but the description should say explain that the conversion is desirable to avoid the cost of starting a subshell as the original test does not suffer from the problem described in your commit message. Best Wishes Phillip > > Signed-off-by: Charvi Mendiratta <charvi077@gmail.com> > --- > t/t7201-co.sh | 10 ++-------- > 1 file changed, 2 insertions(+), 8 deletions(-) > > diff --git a/t/t7201-co.sh b/t/t7201-co.sh > index 74553f991b..5898182fd2 100755 > --- a/t/t7201-co.sh > +++ b/t/t7201-co.sh > @@ -339,10 +339,7 @@ test_expect_success 'switch branches while in subdirectory' ' > git checkout master && > > mkdir subs && > - ( > - cd subs && > - git checkout side > - ) && > + git -C subs checkout side && > ! test -f subs/one && > rm -fr subs > ' > @@ -357,10 +354,7 @@ test_expect_success 'checkout specific path while in subdirectory' ' > > git checkout master && > mkdir -p subs && > - ( > - cd subs && > - git checkout side -- bero > - ) && > + git -C subs checkout side -- bero && > test -f subs/bero > ' > >
On Sun, 18 Oct 2020 at 21:09, Phillip Wood <phillip.wood123@gmail.com> wrote: > > Hi Charvi > > Congratulations on posting your first patch series. > > On 17/10/2020 08:54, Charvi Mendiratta wrote: > > Avoid using `cd` outside of subshells since, if the test fails, there is no guarantee that the current working directory is the expected one, which may cause subsequent tests to run in the wrong directory. > > That is an accurate description of why we want to avoid using `cd` > outside of subshells. However this conversion is converting `cd` inside > a subshell to use `git -C`. I think that is worthwhile as it avoids > having to use a subshell but the description should say explain that the > conversion is desirable to avoid the cost of starting a subshell as the > original test does not suffer from the problem described in your commit > message. > Thank you Philip, for corrections . I somewhat able to understand that commit message should be " avoid using cd inside the subshells " because running a shell script itselfs starts a new subshell, please correct me if I am wrong . But still I am unable to get that why you mentioned the description as "cost of starting a new subshell " . Will this not be the same subshell ? > Best Wishes > > Phillip > > > > > Signed-off-by: Charvi Mendiratta <charvi077@gmail.com> > > --- > > t/t7201-co.sh | 10 ++-------- > > 1 file changed, 2 insertions(+), 8 deletions(-) > > > > diff --git a/t/t7201-co.sh b/t/t7201-co.sh > > index 74553f991b..5898182fd2 100755 > > --- a/t/t7201-co.sh > > +++ b/t/t7201-co.sh > > @@ -339,10 +339,7 @@ test_expect_success 'switch branches while in subdirectory' ' > > git checkout master && > > > > mkdir subs && > > - ( > > - cd subs && > > - git checkout side > > - ) && Is there any specific meaning of writing these above two commands in parentheses . Will this not work the same without it ? > > + git -C subs checkout side && > > ! test -f subs/one && > > rm -fr subs > > ' > > @@ -357,10 +354,7 @@ test_expect_success 'checkout specific path while in subdirectory' ' > > > > git checkout master && > > mkdir -p subs && > > - ( > > - cd subs && > > - git checkout side -- bero > > - ) && > > + git -C subs checkout side -- bero && > > test -f subs/bero > > ' > > > > Thanks and Regards , Charvi
Hi Charvi On 19/10/2020 13:55, Charvi Mendiratta wrote: > On Sun, 18 Oct 2020 at 21:09, Phillip Wood <phillip.wood123@gmail.com> wrote: >> >> Hi Charvi >> >> Congratulations on posting your first patch series. >> >> On 17/10/2020 08:54, Charvi Mendiratta wrote: >>> Avoid using `cd` outside of subshells since, if the test fails, there is no guarantee that the current working directory is the expected one, which may cause subsequent tests to run in the wrong directory. >> >> That is an accurate description of why we want to avoid using `cd` >> outside of subshells. However this conversion is converting `cd` inside >> a subshell to use `git -C`. I think that is worthwhile as it avoids >> having to use a subshell but the description should say explain that the >> conversion is desirable to avoid the cost of starting a subshell as the >> original test does not suffer from the problem described in your commit >> message. >> > > Thank you Philip, for corrections . I somewhat able to understand that > commit message > should be " avoid using cd inside the subshells " because running a > shell script itselfs starts > a new subshell, please correct me if I am wrong . But still I am > unable to get that why you > mentioned the description as "cost of starting a new subshell " . Will > this not be the same subshell ? The original test looks something like ( cd sub && git something ) && The commands between the ( and ) are executed in a subshell, any changes made to the current directory or shell variables in the subshell do not affect the rest of the test script. This is because the subshell starts a separate shell process, but creating this separate process has a cost associated with it. The modified test looks like git -C sub something Here we tell git to change directory before it runs the 'something' command, this is more efficient as we don't need to start any extra processes - there are no subshells. So the purpose of this change is not to "avoid using cd inside a subshell" but to avoid having to use a subshell at all. I hope that helps explain what a subshell is and why we want to avoid using it if we can, do let me know if you want me to clarify anything. Best Wishes Phillip >> Best Wishes >> >> Phillip >> >>> >>> Signed-off-by: Charvi Mendiratta <charvi077@gmail.com> >>> --- >>> t/t7201-co.sh | 10 ++-------- >>> 1 file changed, 2 insertions(+), 8 deletions(-) >>> >>> diff --git a/t/t7201-co.sh b/t/t7201-co.sh >>> index 74553f991b..5898182fd2 100755 >>> --- a/t/t7201-co.sh >>> +++ b/t/t7201-co.sh >>> @@ -339,10 +339,7 @@ test_expect_success 'switch branches while in subdirectory' ' >>> git checkout master && >>> >>> mkdir subs && >>> - ( >>> - cd subs && >>> - git checkout side >>> - ) && > > Is there any specific meaning of writing these above two commands in > parentheses . Will this not work the same without it ? > >>> + git -C subs checkout side && >>> ! test -f subs/one && >>> rm -fr subs >>> ' >>> @@ -357,10 +354,7 @@ test_expect_success 'checkout specific path while in subdirectory' ' >>> >>> git checkout master && >>> mkdir -p subs && >>> - ( >>> - cd subs && >>> - git checkout side -- bero >>> - ) && >>> + git -C subs checkout side -- bero && >>> test -f subs/bero >>> ' >>> >>> > Thanks and Regards , > Charvi >
On Mon, 19 Oct 2020 at 19:16, Phillip Wood <phillip.wood123@gmail.com> wrote: > > Hi Charvi > > On 19/10/2020 13:55, Charvi Mendiratta wrote: > > On Sun, 18 Oct 2020 at 21:09, Phillip Wood <phillip.wood123@gmail.com> wrote: > >> > >> Hi Charvi > >> > >> Congratulations on posting your first patch series. > >> > >> On 17/10/2020 08:54, Charvi Mendiratta wrote: > >>> Avoid using `cd` outside of subshells since, if the test fails, there is no guarantee that the current working directory is the expected one, which may cause subsequent tests to run in the wrong directory. > >> > >> That is an accurate description of why we want to avoid using `cd` > >> outside of subshells. However this conversion is converting `cd` inside > >> a subshell to use `git -C`. I think that is worthwhile as it avoids > >> having to use a subshell but the description should say explain that the > >> conversion is desirable to avoid the cost of starting a subshell as the > >> original test does not suffer from the problem described in your commit > >> message. > >> > > > > Thank you Philip, for corrections . I somewhat able to understand that > > commit message > > should be " avoid using cd inside the subshells " because running a > > shell script itselfs starts > > a new subshell, please correct me if I am wrong . But still I am > > unable to get that why you > > mentioned the description as "cost of starting a new subshell " . Will > > this not be the same subshell ? > > The original test looks something like > ( > cd sub && > git something > ) && > > The commands between the ( and ) are executed in a subshell, any changes > made to the current directory or shell variables in the subshell do not > affect the rest of the test script. This is because the subshell starts > a separate shell process, but creating this separate process has a cost > associated with it. > > The modified test looks like > git -C sub something > > Here we tell git to change directory before it runs the 'something' > command, this is more efficient as we don't need to start any extra > processes - there are no subshells. > > So the purpose of this change is not to "avoid using cd inside a > subshell" but to avoid having to use a subshell at all. > > I hope that helps explain what a subshell is and why we want to avoid > using it if we can, do let me know if you want me to clarify anything. > Yes, thanks a lot Philip I understood the reason. I will do the corrections in commit message and commit body as below : t7201: using 'git -C' to avoid subshell Using 'git-C' instead of 'cd' inside of subshell, to avoid the extra process of starting a new subshell Please confirm, if any other changes are required . > Best Wishes > > Phillip > Thanks and Regards, Charvi > > >> Best Wishes > >> > >> Phillip > >> > >>> > >>> Signed-off-by: Charvi Mendiratta <charvi077@gmail.com> > >>> --- > >>> t/t7201-co.sh | 10 ++-------- > >>> 1 file changed, 2 insertions(+), 8 deletions(-) > >>> > >>> diff --git a/t/t7201-co.sh b/t/t7201-co.sh > >>> index 74553f991b..5898182fd2 100755 > >>> --- a/t/t7201-co.sh > >>> +++ b/t/t7201-co.sh > >>> @@ -339,10 +339,7 @@ test_expect_success 'switch branches while in subdirectory' ' > >>> git checkout master && > >>> > >>> mkdir subs && > >>> - ( > >>> - cd subs && > >>> - git checkout side > >>> - ) && > > > > Is there any specific meaning of writing these above two commands in > > parentheses . Will this not work the same without it ? > > > >>> + git -C subs checkout side && > >>> ! test -f subs/one && > >>> rm -fr subs > >>> ' > >>> @@ -357,10 +354,7 @@ test_expect_success 'checkout specific path while in subdirectory' ' > >>> > >>> git checkout master && > >>> mkdir -p subs && > >>> - ( > >>> - cd subs && > >>> - git checkout side -- bero > >>> - ) && > >>> + git -C subs checkout side -- bero && > >>> test -f subs/bero > >>> ' > >>> > >>> > > Thanks and Regards , > > Charvi > >
Hi Charvi, On Mon, Oct 19, 2020 at 10:54:57PM +0530, Charvi Mendiratta wrote: > Yes, thanks a lot Philip I understood the reason. I will do the corrections in > commit message and commit body as below : > t7201: using 'git -C' to avoid subshell > > Using 'git-C' instead of 'cd' inside of subshell, to avoid the extra process > of starting a new subshell > > Please confirm, if any other changes are required . Usually it never hurts to just send the patch, since any feedback that a reviewer has now is equally good even after you have sent a patch. Plus, it's easier to review the concrete patch you want applied, instead of a hypothetical of what you might send. That said, a couple of notes: - Your subject message is good. It is concise, to-the-point, and accurately describes the change. Good. - The body is similarly short, but could be rewritten to use the imperative mood. But, it is redundant with the subject. The subject says "we are using 'git -C' to avoid creating a subshell", and the patch says exactly the same. ...So, you can do one of two things. Either you can abbreviate the subject, adding the additional detail in the patch message, or you could leave the subject as-is and delete the patch message entirely. Either would be fine with me, but certainly Phillip or others could chime in, too. Thanks, Taylor
On Tue, 20 Oct 2020 at 01:55, Taylor Blau <me@ttaylorr.com> wrote: > > Hi Charvi, > > On Mon, Oct 19, 2020 at 10:54:57PM +0530, Charvi Mendiratta wrote: > > Yes, thanks a lot Philip I understood the reason. I will do the corrections in > > commit message and commit body as below : > > t7201: using 'git -C' to avoid subshell > > > > Using 'git-C' instead of 'cd' inside of subshell, to avoid the extra process > > of starting a new subshell > > > > Please confirm, if any other changes are required . > > Usually it never hurts to just send the patch, since any feedback that a > reviewer has now is equally good even after you have sent a patch. Plus, > it's easier to review the concrete patch you want applied, instead of a > hypothetical of what you might send. > Yes, I completely agree with you . Its my fault, I will send it in the patch and will take care of not repeating this again . > That said, a couple of notes: > > - Your subject message is good. It is concise, to-the-point, and > accurately describes the change. Good. > > - The body is similarly short, but could be rewritten to use the > imperative mood. But, it is redundant with the subject. The subject > says "we are using 'git -C' to avoid creating a subshell", and the > patch says exactly the same. > > ...So, you can do one of two things. Either you can abbreviate the > subject, adding the additional detail in the patch message, or you could > leave the subject as-is and delete the patch message entirely. > Thanks Taylor, I will do the changes as you mentioned and send it in the next patch . > Either would be fine with me, but certainly Phillip or others could > chime in, too. > > Thanks, > Taylor Thanks and Regards, Charvi
Hi Charvi On 19/10/2020 21:25, Taylor Blau wrote: > Hi Charvi, > > On Mon, Oct 19, 2020 at 10:54:57PM +0530, Charvi Mendiratta wrote: >> Yes, thanks a lot Philip I understood the reason. I will do the corrections in >> commit message and commit body as below : >> t7201: using 'git -C' to avoid subshell >> >> Using 'git-C' instead of 'cd' inside of subshell, to avoid the extra process >> of starting a new subshell >> >> [...] > > That said, a couple of notes: > > - Your subject message is good. It is concise, to-the-point, and > accurately describes the change. Good. > > - The body is similarly short, but could be rewritten to use the > imperative mood. But, it is redundant with the subject. The subject > says "we are using 'git -C' to avoid creating a subshell", and the > patch says exactly the same. > > ...So, you can do one of two things. Either you can abbreviate the > subject, adding the additional detail in the patch message, or you could > leave the subject as-is and delete the patch message entirely. > > Either would be fine with me, but certainly Phillip or others could > chime in, too. I'm happy with either, but I would suggest changing the subject so the description starts with 'use' rather than 'using' Best Wishes Phillip > Thanks, > Taylor >
On Tue, 20 Oct 2020 at 14:43, Phillip Wood <phillip.wood123@gmail.com> wrote: > > Hi Charvi > > On 19/10/2020 21:25, Taylor Blau wrote: > > Hi Charvi, > > > > On Mon, Oct 19, 2020 at 10:54:57PM +0530, Charvi Mendiratta wrote: > >> Yes, thanks a lot Philip I understood the reason. I will do the corrections in > >> commit message and commit body as below : > >> t7201: using 'git -C' to avoid subshell > >> > >> Using 'git-C' instead of 'cd' inside of subshell, to avoid the extra process > >> of starting a new subshell > >> > >> [...] > > > > That said, a couple of notes: > > > > - Your subject message is good. It is concise, to-the-point, and > > accurately describes the change. Good. > > > > - The body is similarly short, but could be rewritten to use the > > imperative mood. But, it is redundant with the subject. The subject > > says "we are using 'git -C' to avoid creating a subshell", and the > > patch says exactly the same. > > > > ...So, you can do one of two things. Either you can abbreviate the > > subject, adding the additional detail in the patch message, or you could > > leave the subject as-is and delete the patch message entirely. > > > > Either would be fine with me, but certainly Phillip or others could > > chime in, too. > > I'm happy with either, but I would suggest changing the subject so the > description starts with 'use' rather than 'using' > Thank you Phillip, I have updated the changes in the next patch . > Best Wishes > > Phillip > > > Thanks, > > Taylor > > Thanks and Regards, Charvi
Hi Charvi, On Tue, Oct 20, 2020 at 11:08:55AM +0530, Charvi Mendiratta wrote: > > Usually it never hurts to just send the patch, since any feedback that a > > reviewer has now is equally good even after you have sent a patch. Plus, > > it's easier to review the concrete patch you want applied, instead of a > > hypothetical of what you might send. > > Yes, I completely agree with you . Its my fault, I will send it in the > patch and will > take care of not repeating this again . It's not your fault: there's no fault to be assigned when submitting your first few patches to the list. You're doing much better than I did ;-). > > That said, a couple of notes: > > > > [...] > > > Thanks Taylor, I will do the changes as you mentioned and send it in the > next patch . Thanks! Taylor
diff --git a/t/t7201-co.sh b/t/t7201-co.sh index 74553f991b..5898182fd2 100755 --- a/t/t7201-co.sh +++ b/t/t7201-co.sh @@ -339,10 +339,7 @@ test_expect_success 'switch branches while in subdirectory' ' git checkout master && mkdir subs && - ( - cd subs && - git checkout side - ) && + git -C subs checkout side && ! test -f subs/one && rm -fr subs ' @@ -357,10 +354,7 @@ test_expect_success 'checkout specific path while in subdirectory' ' git checkout master && mkdir -p subs && - ( - cd subs && - git checkout side -- bero - ) && + git -C subs checkout side -- bero && test -f subs/bero '
Avoid using `cd` outside of subshells since, if the test fails, there is no guarantee that the current working directory is the expected one, which may cause subsequent tests to run in the wrong directory. Signed-off-by: Charvi Mendiratta <charvi077@gmail.com> --- t/t7201-co.sh | 10 ++-------- 1 file changed, 2 insertions(+), 8 deletions(-)