Re: [PATCH 2/3] difftool: avoid $GIT_DIR and $GIT_WORK_TREE

From: Junio C Hamano <gitster@pobox.com>
Date: Jul 19, 2016, 18:19 UTC
Message-ID: <xmqqy44xfq3n.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <20160719035756.24961-2-davvid@gmail.com>

David Aguilar <davvid@gmail.com> writes:

Show 6 quoted lines

> Environment variables are global and hard to reason about.
> Use the `--git-dir` and `--work-tree` arguments when invoking `git`
> instead of relying on the environment.
>
> Add a test to ensure that difftool's dir-diff feature works when these
> variables are present in the environment.

Yes, that is a very good idea. I was wondering what would happen with that "must-not-unset-nor-smudge if it already is set" thing.

Show 19 quoted lines

> diff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh
> index 42a2929..fa43c24 100755
> --- a/t/t7800-difftool.sh
> +++ b/t/t7800-difftool.sh
> @@ -412,6 +412,22 @@ run_dir_diff_test 'difftool --dir-diff from subdirectory' '
>  	)
>  '
>  
> +run_dir_diff_test 'difftool --dir-diff from subdirectory with GIT_DIR set' '
> +	(
> +		GIT_DIR=$(pwd)/.git &&
> +		export GIT_DIR &&
> +		GIT_WORK_TREE=$(pwd) &&
> +		export GIT_WORK_TREE &&
> +		cd sub &&
> +		git difftool --dir-diff $symlinks --extcmd ls \
> +			branch -- sub >output &&
> +		sane_unset GIT_WORK_TREE &&
> +		sane_unset GIT_DIR &&

It is not wrong per-se, but as you are in a subshell, you do not have to unset these, I would think. Not worth a reroll, but unless I am overlooking something (in which case please holler) I'm inclined to remove these two lines myself while queuing the series.

Thanks.

> +		grep sub output &&
> +		! grep file output
> +	)
> +'