Re: [RFC PATCH v2 1/3] mergetools: support difftool.tabbed setting

From: Junio C Hamano <gitster@pobox.com>
Date: Jan 18, 2021, 23:25 UTC
Message-ID: <xmqqv9btj09z.fsf@gitster.c.googlers.com>
In-Reply-To: <20210118210003.3071205-2-nicholas@guriev.su>

Nicholas Guriev <nicholas@guriev.su> writes:

Show 13 quoted lines

> diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh
> index 78f3647ed9..2a1edbb9b9 100644
> --- a/git-mergetool--lib.sh
> +++ b/git-mergetool--lib.sh
> @@ -195,6 +195,11 @@ setup_tool () {
>  		false
>  	}
>  
> +	# Clear combo function declared by a previous tool (if any) to
> +	# unambiguously indicate that the current one supports the feature or
> +	# not.
> +	unset -f diff_combo_cmd
> +

I know this is so that you can use "type diff_combo_cmd" in a different part of the patch, but I do not think you want to be fooled by an unrelated ~/bin/diff_combo_cmd that the user may have under the home directory.

Instead, the fallback definitions can have

	diff_combo_supported () {
		return 1
	}

and have backends that does support diff_combo_supported to override with their own

	diff_combo_supported () {
		return 0
	}

And then, the part of is_difftool_tabbed that wants to see if the current backend supports the operation can become:

	test "$GIT_DIFFTOOL_TABBED" = true &&
	test "${GIT_DIFF_PATH_TOTAL=0}" -gt 1 &&
-	type diff_combo_cmd >/dev/null 2>&1
+	diff_combo_supported
 }

That way, you do not have to do "unset -f" up above.

Show 25 quoted lines

> @@ -250,6 +255,28 @@ trust_exit_code () {
>  	fi
>  }
>  
> +# Check whether tabbed mode is requested and current loaded tool supports it.
> +is_difftool_tabbed () {
> +	: "${GIT_DIFFTOOL_TABBED=$(git config --type=bool \
> +		--default=false difftool.tabbed || echo bad value)}"
> +	case $(printf "%s" "$GIT_DIFFTOOL_TABBED" | tr '[:upper:]' '[:lower:]') in
> +	yes|on|true|1)
> +		GIT_DIFFTOOL_TABBED=true
> +		;;
> +	no|off|false|0|'')
> +		GIT_DIFFTOOL_TABBED=false
> +		;;
> +	*)
> +		echo "error: bad boolean value of GIT_DIFFTOOL_TABBED" >&2
> +		exit 1
> +		;;
> +	esac
> +
> +	test "$GIT_DIFFTOOL_TABBED" = true &&
> +	test "${GIT_DIFF_PATH_TOTAL=0}" -gt 1 &&
> +	type diff_combo_cmd >/dev/null 2>&1
> +}