[v5,0/7] difftool and mergetool improvements

Message

Denton Liu April 29, 2019, 6:20 a.m. UTC

Hi David, thanks for the comments.

I'm not really sure why I did the double-negative thing... It seems
obvious that it should be the other way around. I also unrolled the
loops and wrote a gui_mode function.

Good suggestions!

---

Changes since v4:

* Remove double-negative
* Change double-nested search loop into one for-loop
* Create gui_mode function
* Change an instance of "exclusive" to "mutually exclusive"

Changes since v3:

* Move nested for into a subshell so that IFS does not leak out of
  function

Changes since v2:

* Unsuppress output in t7610
* Make `get_merge_tool` return 1 on guessed so we don't have to deal
  with parsing '$guessed:$merge_tool'

Changes since v1:

* Introduce get_merge_tool_guessed function instead of changing
  get_merge_tool
* Remove unnecessary if-tower in mutual exclusivity logic

Denton Liu (7):
  t7610: unsuppress output
  t7610: add mergetool --gui tests
  mergetool: use get_merge_tool function
  mergetool--lib: create gui_mode function
  mergetool: fallback to tool when guitool unavailable
  difftool: make --gui, --tool and --extcmd mutually exclusive
  difftool: fallback on merge.guitool

 Documentation/git-difftool.txt       |   4 +-
 Documentation/git-mergetool--lib.txt |   4 +-
 Documentation/git-mergetool.txt      |   4 +-
 builtin/difftool.c                   |  13 +--
 git-difftool--helper.sh              |   2 +-
 git-mergetool--lib.sh                |  47 ++++++--
 git-mergetool.sh                     |  12 +-
 t/t7610-mergetool.sh                 | 163 +++++++++++++++++----------
 t/t7800-difftool.sh                  |  24 ++++
 9 files changed, 180 insertions(+), 93 deletions(-)

Range-diff against v4:
1:  919aa32e20 = 1:  9f9922cab3 t7610: unsuppress output
2:  9a1bb60b20 = 2:  0f632ca6bf t7610: add mergetool --gui tests
3:  a900ce2a6a ! 3:  81dd25d8e2 mergetool: use get_merge_tool function
    @@ -18,8 +18,9 @@
     
         This change is not completely backwards compatible as there may be
         external users of git-mergetool--lib. However, only one user,
    -    git-diffall[1], was found from searching GitHub and Google. It seems
    -    very unlikely that there exists an external caller that would take into
    +    git-diffall[1], was found from searching GitHub and Google, and this
    +    tool is superseded by `git difftool --dir-diff` anyway. It seems very
    +    unlikely that there exists an external caller that would take into
         account the return code of `get_merge_tool` as it would always return 0
         before this change so this change probably does not affect any external
         users.
    @@ -63,7 +64,7 @@
      }
      
      get_merge_tool () {
    -+	not_guessed=true
    ++	is_guessed=false
      	# Check if a merge tool has been configured
     -	merge_tool=$(get_configured_merge_tool)
     +	merge_tool=$(get_configured_merge_tool $GIT_MERGETOOL_GUI)
    @@ -71,10 +72,10 @@
      	if test -z "$merge_tool"
      	then
      		merge_tool=$(guess_merge_tool) || exit
    -+		not_guessed=false
    ++		is_guessed=true
      	fi
      	echo "$merge_tool"
    -+	test "$not_guessed" = true
    ++	test "$is_guessed" = false
      }
      
      mergetool_find_win32_cmd () {
-:  ---------- > 4:  27a59e1e27 mergetool--lib: create gui_mode function
4:  abcf91688a ! 5:  40413dbda1 mergetool: fallback to tool when guitool unavailable
    @@ -15,8 +15,41 @@
         3. diff.tool
         4. merge.tool
     
    -    Note that the behavior for when difftool or mergetool are called without
    -    `--gui` should be identical with or without this patch.
    +    The behavior for when difftool or mergetool are called without `--gui`
    +    should be identical with or without this patch.
    +
    +    Note that the search loop could be written as
    +
    +            sections="merge"
    +            keys="tool"
    +            if diff_mode
    +            then
    +                    sections="diff $sections"
    +            fi
    +            if gui_mode
    +            then
    +                    keys="guitool $keys"
    +            fi
    +
    +            merge_tool=$(
    +                    IFS=' '
    +                    for key in $keys
    +                    do
    +                            for section in $sections
    +                            do
    +                                    selected=$(git config $section.$key)
    +                                    if test -n "$selected"
    +                                    then
    +                                            echo "$selected"
    +                                            return
    +                                    fi
    +                            done
    +                    done)
    +
    +    which would make adding a mode in the future much easier. However,
    +    adding a new mode will likely never happen as it is highly discouraged
    +    so, as a result, it is written in its current form so that it's
    +    immediately obvious for future readers.
     
         Signed-off-by: Denton Liu <[e-mail]>
     
    @@ -42,41 +75,43 @@
      }
      
      get_configured_merge_tool () {
    --	# If first argument is true, find the guitool instead
    --	if test "$1" = true
    -+	is_gui="$1"
    -+	sections="merge"
    -+	keys="tool"
    -+
    -+	if diff_mode
    - 	then
    +-	if gui_mode
    +-	then
     -		gui_prefix=gui
    -+		sections="diff $sections"
    - 	fi
    - 
    +-	fi
    +-
     -	# Diff mode first tries diff.(gui)tool and falls back to merge.(gui)tool.
     -	# Merge mode only checks merge.(gui)tool
    --	if diff_mode
    -+	if "$is_gui" = true
    ++	keys=
    + 	if diff_mode
      	then
     -		merge_tool=$(git config diff.${gui_prefix}tool || git config merge.${gui_prefix}tool)
    --	else
    ++		if gui_mode
    ++		then
    ++			keys="diff.guitool merge.guitool diff.tool merge.tool"
    ++		else
    ++			keys="diff.tool merge.tool"
    ++		fi
    + 	else
     -		merge_tool=$(git config merge.${gui_prefix}tool)
    -+		keys="guitool $keys"
    ++		if gui_mode
    ++		then
    ++			keys="merge.guitool merge.tool"
    ++		else
    ++			keys="merge.tool"
    ++		fi
      	fi
     +
     +	merge_tool=$(
     +		IFS=' '
     +		for key in $keys
     +		do
    -+			for section in $sections
    -+			do
    -+				if selected=$(git config $section.$key)
    -+				then
    -+					echo "$selected"
    -+					return
    -+				fi
    -+			done
    ++			selected=$(git config $key)
    ++			if test -n "$selected"
    ++			then
    ++				echo "$selected"
    ++				return
    ++			fi
     +		done)
     +
      	if test -n "$merge_tool" && ! valid_tool "$merge_tool"
5:  9ec39c5af0 ! 6:  c70789b689 difftool: make --gui, --tool and --extcmd mutually exclusive
    @@ -29,7 +29,7 @@
      	test_cmp expect actual
      '
      
    -+test_expect_success 'difftool --gui, --tool and --extcmd are exclusive' '
    ++test_expect_success 'difftool --gui, --tool and --extcmd are mutually exclusive' '
     +	difftool_test_setup &&
     +	test_must_fail git difftool --gui --tool=test-tool &&
     +	test_must_fail git difftool --gui --extcmd=cat &&
6:  a72009fc3d = 7:  3fd4f46a7c difftool: fallback on merge.guitool