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

[Markus Heidelberg <markus.heidelberg@web.de>]

David Aguilar, 06.04.2009:
> diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh

> +check_unchanged () {
> +	if merge_mode; then
> +		if test "$MERGED" -nt "$BACKUP"; then
> +			status=0
> +		else
> +			while true; do
> +				echo "$MERGED seems unchanged."
> +				printf "Was the merge successful? [y/n] "
> +				read answer < /dev/tty
> +				case "$answer" in
> +				y*|Y*) status=0; break ;;
> +				n*|N*) status=1; break ;;
> +				esac
> +			done
> +		fi
> +	else
> +		status=0
> +	fi
> +	return $status
> +}

The return value is currently never used.

> +run_merge_tool () {
> +	base_present="$2"
> +	if diff_mode; then
> +		base_present="false"
> +	fi
> +	if test -z "$base_present"; then
> +		base_present="true"
> +	fi

The second if is never true, so isn't necessary. run_merge_tool() is
called with $2 = true or false in mergetool and $2 = "" in difftool.

But I wonder, if it would be better to change the proceeding in the
case-esac in the next hunk below:

Currently it is:
    if $base_present
        mergetool with base
    else
        if $merge_mode
            mergetool without base
        else
            difftool
        fi
    fi

Maybe better:
    if $merge_mode
        if $base_present
            mergetool with base
        else
            mergetool without base
        fi
    else
        difftool
    fi

Then the first if can vanish as well and $base_present doesn't have to
be set to false in diff_mode.

And check_unchanged() doesn't have to be called in diff_mode any more,
$status could be set to 0 by default and doesn't have to be touched when
in diff_mode. Only in merge_mode git-mergetool has to know, whether the
merge went fine.

Then it will be:
    if $merge_mode
        touch $BACKUP
        if $base_present
            mergetool with base
        else
            mergetool without base
        fi
	check_unchanged
    else
        difftool
    fi

or:
    if $merge_mode
        if $base_present
            mergetool with base
        else
            mergetool without base
        fi
	status=$?
    else
        difftool
    fi

Sorry for coming so late with this.

> +	case "$1" in
> +	kdiff3)
> +		if $base_present; then
> +			("$merge_tool_path" --auto \
> +			 --L1 "$MERGED (Base)" --L2 "$MERGED (Local)" --L3 "$MERGED (Remote)" \
> +			 -o "$MERGED" "$BASE" "$LOCAL" "$REMOTE" > /dev/null 2>&1)
> +		else
> +			if merge_mode; then
> +				("$merge_tool_path" --auto \
> +				 --L1 "$MERGED (Local)" \
> +				 --L2 "$MERGED (Remote)" \
> +				 -o "$MERGED" "$LOCAL" "$REMOTE" \
> +				> /dev/null 2>&1)
> +			else
> +				("$merge_tool_path" --auto \
> +				 --L1 "$MERGED (A)" \
> +				 --L2 "$MERGED (B)" \
> +				 "$LOCAL" "$REMOTE" \
> +				> /dev/null 2>&1)
> +			fi
> +		fi
> +		status=$?
> +		;;
> +	kompare)
> +		"$merge_tool_path" "$LOCAL" "$REMOTE"
> +		status=$?
> +		;;
> +	tkdiff)
> +		if $base_present; then
> +			"$merge_tool_path" -a "$BASE" -o "$MERGED" "$LOCAL" "$REMOTE"
> +		else
> +			if merge_mode; then
> +				"$merge_tool_path" -o "$MERGED" "$LOCAL" "$REMOTE"
> +			else
> +				"$merge_tool_path" "$LOCAL" "$REMOTE"
> +			fi
> +		fi
> +		status=$?
> +		;;
> +	meld)
> +		if merge_mode; then
> +			touch "$BACKUP"
> +			"$merge_tool_path" "$LOCAL" "$MERGED" "$REMOTE"
> +		else
> +			"$merge_tool_path" "$LOCAL" "$REMOTE"
> +		fi
> +		check_unchanged
> +		;;
> +	diffuse)
> +		if merge_mode; then
> +			touch "$BACKUP"
> +		fi
> +		if $base_present; then
> +			"$merge_tool_path" "$LOCAL" "$MERGED" "$REMOTE" "$BASE" | cat
> +		else
> +			if merge_mode; then
> +				"$merge_tool_path" "$LOCAL" "$MERGED" "$REMOTE" | cat
> +			else
> +				"$merge_tool_path" "$LOCAL" "$REMOTE" | cat
> +			fi
> +		fi
> +		check_unchanged
> +		;;
> +	vimdiff)
> +		if merge_mode; then
> +			touch "$BACKUP"
> +			"$merge_tool_path" -d -c "wincmd l" "$LOCAL" "$MERGED" "$REMOTE"
> +			check_unchanged
> +		else
> +			"$merge_tool_path" -d -c "wincmd l" "$LOCAL" "$REMOTE"
> +		fi
> +		;;
> +	gvimdiff)
> +		if merge_mode; then
> +			touch "$BACKUP"
> +			"$merge_tool_path" -d -c "wincmd l" -f "$LOCAL" "$MERGED" "$REMOTE"
> +			check_unchanged
> +		else
> +			"$merge_tool_path" -d -c "wincmd l" -f "$LOCAL" "$REMOTE"
> +		fi
> +		;;
> +	xxdiff)
> +		if merge_mode; then
> +			touch "$BACKUP"
> +		fi
> +		if $base_present; then
> +			"$merge_tool_path" -X --show-merged-pane \
> +			    -R 'Accel.SaveAsMerged: "Ctrl-S"' \
> +			    -R 'Accel.Search: "Ctrl+F"' \
> +			    -R 'Accel.SearchForward: "Ctrl-G"' \
> +			    --merged-file "$MERGED" "$LOCAL" "$BASE" "$REMOTE"
> +		else
> +			if merge_mode; then
> +				"$merge_tool_path" -X $extra \
> +					-R 'Accel.SaveAsMerged: "Ctrl-S"' \
> +					-R 'Accel.Search: "Ctrl+F"' \
> +					-R 'Accel.SearchForward: "Ctrl-G"' \
> +					--merged-file "$MERGED" "$LOCAL" "$REMOTE"
> +			else
> +				"$merge_tool_path" \
> +					-R 'Accel.Search: "Ctrl+F"' \
> +					-R 'Accel.SearchForward: "Ctrl-G"' \
> +					"$LOCAL" "$REMOTE"
> +			fi
> +		fi
> +		check_unchanged
> +		;;
> +	opendiff)
> +		merge_mode && touch "$BACKUP"
> +		if $base_present; then
> +			"$merge_tool_path" "$LOCAL" "$REMOTE" \
> +				-ancestor "$BASE" -merge "$MERGED" | cat
> +		else
> +			if merge_mode; then
> +				"$merge_tool_path" "$LOCAL" "$REMOTE" \
> +					-merge "$MERGED" | cat
> +			else
> +				"$merge_tool_path" "$LOCAL" "$REMOTE" | cat
> +			fi
> +		fi
> +		check_unchanged
> +		;;
> +	ecmerge)
> +		merge_mode && touch "$BACKUP"
> +		if $base_present; then
> +			"$merge_tool_path" "$BASE" "$LOCAL" "$REMOTE" \
> +				--default --mode=merge3 --to="$MERGED"
> +		else
> +			"$merge_tool_path" "$LOCAL" "$REMOTE" \
> +				--default --mode=merge2 --to="$MERGED"
> +		fi
> +		check_unchanged
> +		;;
> +	emerge)
> +		if $base_present; then
> +			"$merge_tool_path" -f emerge-files-with-ancestor-command \
> +				"$LOCAL" "$REMOTE" "$BASE" "$(basename "$MERGED")"
> +		else
> +			"$merge_tool_path" -f emerge-files-command \
> +				"$LOCAL" "$REMOTE" "$(basename "$MERGED")"
> +		fi
> +		status=$?
> +		;;
> +	tortoisemerge)
> +		if $base_present; then
> +			touch "$BACKUP"
> +			"$merge_tool_path" -base:"$BASE" -mine:"$LOCAL" -theirs:"$REMOTE" -merged:"$MERGED"
> +			check_unchanged
> +		else
> +			echo "TortoiseMerge cannot be used without a base" 1>&2
> +			status=1
> +		fi
> +		;;
> +	*)
> +		if test -n "$merge_tool_cmd"; then
> +			if merge_mode &&
> +			test "$merge_tool_trust_exit_code" = "false"; then
> +				touch "$BACKUP"
> +				( eval $merge_tool_cmd )
> +				check_unchanged
> +			else
> +				( eval $merge_tool_cmd )
> +				status=$?
> +			fi
> +		fi
> +		;;
> +	esac
> +	return $status
> +}
> +
> +guess_merge_tool () {
> +	if diff_mode; then
> +		kompare="kompare"
> +	fi
> +	if test -n "$DISPLAY"; then
> +		if test -n "$GNOME_DESKTOP_SESSION_ID" ; then
> +			tools="meld kdiff3 $kompare tkdiff"
> +			tools="$tools xxdiff gvimdiff diffuse"
> +		else
> +			tools="kdiff3 $kompare tkdiff xxdiff"
> +			tools="$tools meld gvimdiff diffuse"
> +		fi
> +	fi

We lost tortoisemerge. And we could add ecmerge. Then every possible
built-in can be detected if installed.

> +	if echo "${VISUAL:-$EDITOR}" | grep emacs > /dev/null 2>&1; then
> +		# $EDITOR is emacs so add emerge as a candidate
> +		tools="$tools emerge opendiff vimdiff"
> +	elif echo "${VISUAL:-$EDITOR}" | grep vim > /dev/null 2>&1; then
> +		# $EDITOR is vim so add vimdiff as a candidate
> +		tools="$tools vimdiff opendiff emerge"
> +	else
> +		tools="$tools opendiff emerge vimdiff"
> +	fi

Why is opendiff here? I thought the graphical tools should go above.
Doesn't have Mac OS $DISPLAY set?