Git Mailing List Archive on lore.kernel.org
 help / color / Atom feed
From: Phillip Wood <phillip.wood123@gmail.com>
To: Charvi Mendiratta <charvi077@gmail.com>, phillip.wood@dunelm.org.uk
Cc: git <git@vger.kernel.org>, Junio C Hamano <gitster@pobox.com>,
	Christian Couder <christian.couder@gmail.com>,
	Eric Sunshine <sunshine@sunshineco.com>
Subject: Re: [PATCH v2 4/5][Outreachy] t7201: avoid using cd outside of subshells
Date: Mon, 19 Oct 2020 14:46:00 +0100
Message-ID: <3b501a3a-b675-3eb7-975a-cc9206f15057@gmail.com> (raw)
In-Reply-To: <CAPSFM5ejRWUc2mCtqTPH4a6Q-WWUC4mQHU=bsHkjJOdG4kwW0g@mail.gmail.com>

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
> 

  reply index

Thread overview: 60+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-10-15 17:57 [PATCH 0/5][Outreachy] modernizing the test scripts charvi-077
2020-10-15 17:57 ` [PATCH 1/5][Outreachy] t7101,t7102,t7201: modernize test formatting charvi-077
2020-10-16 13:07   ` Christian Couder
2020-10-15 17:57 ` [PATCH 2/5][Outreachy] t7102,t7201: remove unnecessary blank spaces in test body charvi-077
2020-10-15 17:57 ` [PATCH 3/5][Outreachy] t7102,t7201: remove whitespace after redirect operator charvi-077
2020-10-15 17:57 ` [PATCH 4/5][Outreachy] t7201: avoid using cd outside of subshells charvi-077
2020-10-15 17:57 ` [PATCH 5/5][Outreachy] t7201: place each command in its own line charvi-077
2020-10-16 12:54 ` [PATCH 0/5][Outreachy] modernizing the test scripts Christian Couder
2020-10-17  8:27   ` Charvi Mendiratta
2020-10-17  7:54 ` [PATCH v2 " Charvi Mendiratta
2020-10-17  7:54   ` [PATCH v2 1/5][Outreachy] t7101,t7102,t7201: modernize test formatting Charvi Mendiratta
2020-10-17  7:54   ` [PATCH v2 2/5][Outreachy] t7102,t7201: remove unnecessary blank spaces in test body Charvi Mendiratta
2020-10-17 15:13     ` Đoàn Trần Công Danh
2020-10-18  5:40       ` Charvi Mendiratta
2020-10-17  7:54   ` [PATCH v2 3/5][Outreachy] t7102,t7201: remove whitespace after redirect operator Charvi Mendiratta
2020-10-17  7:54   ` [PATCH v2 4/5][Outreachy] t7201: avoid using cd outside of subshells Charvi Mendiratta
2020-10-18 15:39     ` Phillip Wood
2020-10-19 12:55       ` Charvi Mendiratta
2020-10-19 13:46         ` Phillip Wood [this message]
2020-10-19 17:24           ` Charvi Mendiratta
2020-10-19 20:25             ` Taylor Blau
2020-10-20  5:38               ` Charvi Mendiratta
2020-10-20 20:09                 ` Taylor Blau
2020-10-20  9:13               ` Phillip Wood
2020-10-20 11:48                 ` Charvi Mendiratta
2020-10-17  7:54   ` [PATCH v2 5/5][Outreachy] t7201: place each command in its own line Charvi Mendiratta
2020-10-20 11:43   ` [PATCH v3 0/5][Outreachy] modernize the test scripts Charvi Mendiratta
2020-10-20 11:43     ` [PATCH v3 1/5][Outreachy] t7101,t7102,t7201: modernize test formatting Charvi Mendiratta
2020-10-20 11:43     ` [PATCH v3 2/5][Outreachy] t7102,t7201: remove unnecessary blank spaces in test body Charvi Mendiratta
2020-10-20 11:43     ` [PATCH v3 3/5][Outreachy] t7102,t7201: remove whitespace after redirect operator Charvi Mendiratta
2020-10-20 11:43     ` [PATCH v3 4/5][Outreachy] t7201: use 'git -C' to avoid subshell Charvi Mendiratta
2020-10-20 11:43     ` [PATCH v3 5/5][Outreachy] t7201: put each command on a seperate line Charvi Mendiratta
2020-10-20 12:11   ` [PATCH v4] t7201: put each command on a separate line Charvi Mendiratta
2020-10-20 20:13     ` Junio C Hamano
2020-10-20 20:15       ` Taylor Blau
2020-10-20 20:25         ` Junio C Hamano
2020-10-20 20:30           ` Taylor Blau
2020-10-20 21:00             ` Junio C Hamano
2020-10-21  7:14             ` Charvi Mendiratta
2020-10-20 20:19       ` Junio C Hamano
2020-10-21 13:16         ` Charvi Mendiratta
2020-10-21 12:48     ` [PATCH v5 0/5][Outreachy] modernize the test scripts Charvi Mendiratta
2020-10-21 12:48       ` [PATCH v5 1/5][Outreachy] t7101,t7102,t7201: modernize test formatting Charvi Mendiratta
2020-10-21 12:48       ` [PATCH v5 2/5][Outreachy] t7102,t7201: remove unnecessary blank spaces in test body Charvi Mendiratta
2020-10-21 12:48       ` [PATCH v5 3/5][Outreachy] t7102,t7201: remove whitespace after redirect operator Charvi Mendiratta
2020-10-21 17:20         ` Eric Sunshine
2020-10-22  5:44           ` Junio C Hamano
2020-10-22  5:53             ` Eric Sunshine
2020-10-22  5:55             ` Junio C Hamano
2020-10-22  6:04               ` Eric Sunshine
2020-10-22 17:35                 ` Junio C Hamano
2020-10-22  6:29             ` Charvi Mendiratta
2020-10-21 12:48       ` [PATCH v5 4/5][Outreachy] t7201: use 'git -C' to avoid subshell Charvi Mendiratta
2020-10-21 12:48       ` [PATCH v5 5/5][Outreachy] t7201: put each command on a separate line Charvi Mendiratta
2020-10-22  7:16       ` [PATCH v6 0/5][Outreachy] modernize test scripts Charvi Mendiratta
2020-10-22  7:16       ` [PATCH v6 1/5][Outreachy] t7101,t7102,t7201: modernize test formatting Charvi Mendiratta
2020-10-22  7:16       ` [PATCH v6 2/5][Outreachy] t7102,t7201: remove unnecessary blank spaces in test body Charvi Mendiratta
2020-10-22  7:16       ` [PATCH v6 3/5][Outreachy] t7102,t7201: remove whitespace after redirect operator Charvi Mendiratta
2020-10-22  7:16       ` [PATCH v6 4/5][Outreachy] t7201: use 'git -C' to avoid subshell Charvi Mendiratta
2020-10-22  7:16       ` [PATCH v6 5/5][Outreachy] t7201: put each command on a separate line Charvi Mendiratta

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=3b501a3a-b675-3eb7-975a-cc9206f15057@gmail.com \
    --to=phillip.wood123@gmail.com \
    --cc=charvi077@gmail.com \
    --cc=christian.couder@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=phillip.wood@dunelm.org.uk \
    --cc=sunshine@sunshineco.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Git Mailing List Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/git/0 git/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 git git/ https://lore.kernel.org/git \
		git@vger.kernel.org
	public-inbox-index git

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.git


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git