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

From: David Aguilar <davvid@gmail.com>
Date: Apr 12, 2009, 03:41 UTC
Message-ID: <20090412034127.GA1398@gmail.com>
In-Reply-To: <20090411120040.GA10381@hashpling.org>

On  0, Charles Bailey <charles@hashpling.org> wrote:

Show 19 quoted lines

> On Wed, Apr 08, 2009 at 12:17:20AM -0700, David Aguilar wrote:
> 
> Finally, I've got around to some git work...
> 
> > diff --git a/Documentation/git-mergetool--lib.txt
> --- snip ---
> > +
> > +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.

Done. My new patch further removes those pesky globals ;)

Show 30 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 ---
> 
> > +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

mergetool.$tool.path

Show 19 quoted lines

> 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.

mergetool.$tool.path does work (there's a test for that feature, though we only test difftool.$tool.path currently)

What you probably ran into was that get_merge_tool_path() had a side-effect of setting $merge_tool_path and thus would just return that value the 2nd time you ran it.

If you started from a fresh shell (instead of faking X11 by setting $DISPLAY) then it would behave correctly.

In any case, yes, it needs to be more robust, so I'm addressing that in: "mergetool--lib: simplify API usage by removing more global variables" (patch on its way)

Show 16 quoted lines

> --- 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

Definitely. This was cleaned up as well.

In my patch get_merge_tool_path and get_merge_tool_cmd now take $1 as an optional argument since they can easily look up the $merge_tool themselves in lieu of a passed-in value.

I kept thinking of mergetool--lib from the POV of difftool and mergetool, but these changes are good since they make it more useful from a standalone POV.

Let me know what you think.

-- 
	David