Re: [PATCH v3] tests: use "env" to run commands with temporary env-var settings

[Junio C Hamano]

David Tran  writes:

> Originally, we would use "VAR=VAL command" to execute a test command with
> environment variable(s) only for that command. This does not work for commands
> that are shell functions (most notably test functions like "test_must_fail");
> the result of the assignment is retained and affects later commands.
>
> To avoid this, we assigned and exported the environment variables and run
> the test(s) in a subshell like this,
>
>   (
>   VAR=VAL &&
>   export VAR
>   test_must_fail git command to be tested
>   )
>
> Using the "env" utility, we should be able to say
>
>   test_must_fail git command to be tested
>
> which is much short and easier to read.

Looks familiar ;-) but it seems the changes from the original you
took it from all look worsening, not improvements, to me.

>>Isn't GIT_CONFIG here another way of saying:
>>
>>  test_must_fail git config -f doesnotexist --list
>>
>>Perhaps that is shorter and more readable still (and there are a few
>>similar cases in this patch.
> I'll ignore this for now. If needed I can make another patch to resolve this.

Yes, I think that is sensible.  And it does not have to be done by you.

> Hopefully this should be all of it.

Seems to be well done.  Thanks.

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3] tests: use "env" to run commands with temporary env-var settings

[Eric Sunshine]

On Tue, Mar 18, 2014 at 2:54 PM, David Tran  wrote:
> Originally, we would use "VAR=VAL command" to execute a test command with
> environment variable(s) only for that command. This does not work for commands
> that are shell functions (most notably test functions like "test_must_fail");
> the result of the assignment is retained and affects later commands.
>
> To avoid this, we assigned and exported the environment variables and run
> the test(s) in a subshell like this,
>
> (
> VAR=VAL &&
> export VAR

Append && to this line.

> test_must_fail git command to be tested
> )
>
> Using the "env" utility, we should be able to say
>
> test_must_fail git command to be tested
>
> which is much short and easier to read.

s/short/shorter/

> Signed-off-by: David Tran 
>
> ---
>
> Hopefully this should be all of it.

Much better. I didn't spot any errors in the patch this time around.

One final note for future submissions: As a courtesy to reviewers,
explain (below the "---" line) what changed in the current version,
and provide a reference to the previous attempt, like this [1].

[1]: http://thread.gmane.org/gmane.comp.version-control.git/244379

> Signed-off-by: David Tran 
> ---
>  t/t1300-repo-config.sh|   17 ++
>  t/t1510-repo-setup.sh |4 +--
>  t/t3200-branch.sh |   12 +--
>  t/t3301-notes.sh  |   22 +++-
>  t/t3404-rebase-interactive.sh |   69 
> -
>  t/t3413-rebase-hook.sh|6 +---
>  t/t4014-format-patch.sh   |   14 ++--
>  t/t5305-include-tag.sh|4 +--
>  t/t5602-clone-remote-exec.sh  |   13 ++--
>  t/t5801-remote-helpers.sh |6 +--
>  t/t6006-rev-list-format.sh|9 ++---
>  t/t7006-pager.sh  |   18 ++-
>  12 files changed, 42 insertions(+), 152 deletions(-)
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html