From: David Aguilar <davvid@gmail.com>
Date: Thu, 21 Jan 2010 07:44:28 GMT
Subject: Re: [PATCH 1/2] Allow to override mergetool.prompt with $GIT_MERGETOOL*_PROMPT
Message-ID: <20100121074427.GA26973@gmail.com>
In-Reply-To: <4B572192.2020606@gmail.com>

Hi,

On Wed, Jan 20, 2010 at 04:30:26PM +0100, Sebastian Schuberth wrote:
> This is for symmetry with git-difftool, and to make should_prompt_merge ()
> reusable from within git-difftool--helper.
> 
> Signed-off-by: Sebastian Schuberth <sschuberth@gmail.com>
> ---
>  git-mergetool--lib.sh |    9 +++++++++
>  git-mergetool.sh      |    3 ++-
>  2 files changed, 11 insertions(+), 1 deletions(-)
> 
> diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh
> index 5b62785..16b0343 100644
> --- a/git-mergetool--lib.sh
> +++ b/git-mergetool--lib.sh
> @@ -7,6 +7,15 @@ merge_mode() {
>  	test "$TOOL_MODE" = merge
>  }
>  
> +should_prompt_merge () {
> +	local prompt=$(git config --bool mergetool.prompt || echo true)
> +	if test "$prompt" = true; then
> +		test -z "$GIT_MERGETOOL_NO_PROMPT"
> +	else
> +		test -n "$GIT_MERGETOOL_PROMPT"
> +	fi
> +}

Someone already mentioned dropping "local" here.

The GIT_DIFFTOOL*_PROMPT variables are implementation details
about how git-difftool passes options to git-difftool--helper.
We don't advertise them in the documentation so we probably
shouldn't support them aside from what is needed for
git-difftool.  We can drop this part.

61ed71dc just recently removed git-difftool--helper's use of
GIT_MERGE_TOOL based on this rationale.


> diff --git a/git-mergetool.sh b/git-mergetool.sh
> index b52a741..d453cb0 100755
> --- a/git-mergetool.sh
> +++ b/git-mergetool.sh
> @@ -202,7 +202,8 @@ merge_file () {
>      return 0
>  }
>  
> -prompt=$(git config --bool mergetool.prompt || echo true)
> +should_prompt_merge
> +prompt=$?
>  
>  while test $# != 0
>  do

git-difftool falling back to mergetool.prompt when
difftool.prompt is not available is good, especially
since we advertise that feature.

Once you drop the local declarations and the env. variable
it should be good to go.  Patch v2, soon?

-- 

	David