On 0, Charles Bailey <charles@xxxxxxxxxxxxx> wrote:
> 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 ;)
> > 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
> 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)
> --- 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html