Re: [PATCH v6 14/14] difftool/mergetool: refactor commands to use git-mergetool--lib

From: Charles Bailey <charles@hashpling.org>
Date: Apr 11, 2009, 12:00 UTC
Message-ID: <20090411120040.GA10381@hashpling.org>
In-Reply-To: <1239175040-28974-1-git-send-email-davvid@gmail.com>

On Wed, Apr 08, 2009 at 12:17:20AM -0700, David Aguilar wrote:

Finally, I've got around to some git work...

Show 6 quoted lines

> diff --git a/Documentation/git-mergetool--lib.txt
> b/Documentation/git-mergetool--lib.txt
> new file mode 100644
> index 0000000..3d57031
> --- /dev/null
> +++ b/Documentation/git-mergetool--lib.txt

--- snip ---

Show 8 quoted lines

> +
> +run_merge_tool::
> +	launches a merge tool given the tool name and a true/false
> +	flag to indicate whether a merge base is present.
> +	'$merge_tool', '$merge_tool_path', and for custom commands,
> +	'$merge_tool_cmd', must be defined prior to calling
> +	run_merge_tool.  Additionally, '$MERGED', '$LOCAL', '$REMOTE',
> +	and '$BASE' must be defined for use by the merge tool.

Reading this function it doesn't look like the variable $merge_tool is actually used, only the first parameter which should be the same thing. Also, TOOL_MODE is used and is important so should probably be documented here instead.

Show 5 quoted lines

> diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh
> new file mode 100644
> index 0000000..c5db24e
> --- /dev/null
> +++ b/git-mergetool--lib.sh

--- snip ---

Show 19 quoted lines

> +get_merge_tool_path () {
> +	# A merge tool has been set, so verify that it's valid.
> +	if ! valid_tool "$merge_tool"; then
> +		echo >&2 "Unknown merge tool $merge_tool"
> +		exit 1
> +	fi
> +	if diff_mode; then
> +		merge_tool_path=$(git config difftool."$merge_tool".path)
> +	fi
> +	if test -z "$merge_tool_path"; then
> +		merge_tool_path=$(git config mergetool."$merge_tool".path)
> +	fi
> +	merge_tool_path="$(translate_merge_tool_path "$merge_tool" "$merge_tool_path")"
> +	if test -z "$merge_tool_cmd" && ! type "$merge_tool_path" > /dev/null 2>&1; then
> +		echo >&2 "The $TOOL_MODE tool $merge_tool is not available as '$merge_tool_path'"
> +		exit 1
> +	fi
> +	echo "$merge_tool_path"
> +}

I just wanted to double check that we supported finding tools in the path with merge.$tool.path unset (it should just work), so I had a quick play and got some really wierd results. Please tell me if just sourcing git-mergetool--lib isn't just going to work.

>From a non-X session:

    $ git config -l | grep 'merge\.\?tool'; get_merge_tool; get_merge_tool_path
    mergetool.p4merge.cmd=echo /"$BASE"/ /"$REMOTE"/ /"$LOCAL"/ /"$MERGED"/
    merge tool candidates: kompare emerge vimdiff
    vimdiff
    vim

>From an X session:

    mergetool.p4merge.cmd=echo /"$BASE"/ /"$REMOTE"/ /"$LOCAL"/ /"$MERGED"/
    merge tool candidates: meld opendiff kdiff3 tkdiff xxdiff kompare gvimdiff diffuse ecmerge emerge vimdiff
    kdiff3
    vim

I'm not quite sure what's going on here.

> diff --git a/git-mergetool.sh b/git-mergetool.sh
> index cceebb7..efa31a2 100755
> --- a/git-mergetool.sh
> +++ b/git-mergetool.sh

--- big snip to: @@ merge_file () {

> +    present=false
> +    base_present &&
> +    present=true

I'm not sure why, but from a style point of view, this seemed a bit inconsistent from the rest of mergetool and grated with me a bit.

I think I'd prefer

if base_present; then
    present=false
else
    present=true
fi

or even:

present=$(base_present && echo true || echo false)

Really, really minor stlye point, though.

-- 
Charles Bailey
http://ccgi.hashpling.plus.com/blog/