* [PATCH 0/4] Various merge / diff tool related minor clean-ups and improvements @ 2012-07-23 7:10 Sebastian Schuberth 2012-07-23 7:14 ` [PATCH v2 0/5] " Sebastian Schuberth 0 siblings, 1 reply; 40+ messages in thread From: Sebastian Schuberth @ 2012-07-23 7:10 UTC (permalink / raw) To: git; +Cc: David Aguilar This series introduce various minor clean-ups and improvements to the merge / diff tool scripts and documentation. Sebastian Schuberth (4): Use variables for the lists of tools that support merging / diffing Explicitly list all valid diff tools and document --tool-help as an option Make sure to use Araxis' "compare" and not e.g. ImageMagick's Add a few more code comments and blank lines in guess_merge_tool Documentation/git-difftool.txt | 9 ++++++--- contrib/completion/git-completion.bash | 11 +++++++++-- git-mergetool--lib.sh | 6 ++++++ mergetools/araxis | 8 +++++++- 4 files changed, 28 insertions(+), 6 deletions(-) -- 1.7.11.msysgit.2 ^ permalink raw reply [flat|nested] 40+ messages in thread
* [PATCH v2 0/5] Various merge / diff tool related minor clean-ups and improvements 2012-07-23 7:10 [PATCH 0/4] Various merge / diff tool related minor clean-ups and improvements Sebastian Schuberth @ 2012-07-23 7:14 ` Sebastian Schuberth 2012-07-23 7:17 ` [PATCH v2 1/5] Sort the list of tools that support both merging and diffing alphabetically Sebastian Schuberth ` (4 more replies) 0 siblings, 5 replies; 40+ messages in thread From: Sebastian Schuberth @ 2012-07-23 7:14 UTC (permalink / raw) Cc: git, David Aguilar This series introduces various minor clean-ups and improvements to the merge / diff tool scripts and documentation. Sorry, the first version was missing a patch. Sebastian Schuberth (5): Sort the list of tools that support both merging and diffing alphabetically Use variables for the lists of tools that support merging / diffing Explicitly list all valid diff tools and document --tool-help as an option Make sure to use Araxis' "compare" and not e.g. ImageMagick's Add a few more code comments and blank lines in guess_merge_tool Documentation/git-difftool.txt | 9 ++++++--- contrib/completion/git-completion.bash | 15 +++++++++++---- git-mergetool--lib.sh | 6 ++++++ mergetools/araxis | 8 +++++++- 4 files changed, 30 insertions(+), 8 deletions(-) -- 1.7.11.msysgit.2 ^ permalink raw reply [flat|nested] 40+ messages in thread
* [PATCH v2 1/5] Sort the list of tools that support both merging and diffing alphabetically 2012-07-23 7:14 ` [PATCH v2 0/5] " Sebastian Schuberth @ 2012-07-23 7:17 ` Sebastian Schuberth 2012-07-23 7:18 ` [PATCH v2 2/5] Use variables for the lists of tools that support merging / diffing Sebastian Schuberth ` (3 subsequent siblings) 4 siblings, 0 replies; 40+ messages in thread From: Sebastian Schuberth @ 2012-07-23 7:17 UTC (permalink / raw) To: git; +Cc: David Aguilar Signed-off-by: Sebastian Schuberth <sschuberth@gmail.com> --- contrib/completion/git-completion.bash | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash index 5be9dee..f2c4894 100755 --- a/contrib/completion/git-completion.bash +++ b/contrib/completion/git-completion.bash @@ -1325,8 +1325,8 @@ _git_diff () __git_complete_revlist_file } -__git_mergetools_common="diffuse ecmerge emerge kdiff3 meld opendiff - tkdiff vimdiff gvimdiff xxdiff araxis p4merge bc3 +__git_mergetools_common="araxis bc3 diffuse ecmerge emerge gvimdiff + kdiff3 meld opendiff p4merge tkdiff vimdiff xxdiff " _git_difftool () -- 1.7.11.msysgit.2 ^ permalink raw reply related [flat|nested] 40+ messages in thread
* [PATCH v2 2/5] Use variables for the lists of tools that support merging / diffing 2012-07-23 7:14 ` [PATCH v2 0/5] " Sebastian Schuberth 2012-07-23 7:17 ` [PATCH v2 1/5] Sort the list of tools that support both merging and diffing alphabetically Sebastian Schuberth @ 2012-07-23 7:18 ` Sebastian Schuberth 2012-07-23 16:46 ` Junio C Hamano 2012-07-23 7:18 ` [PATCH 3/5] Explicitly list all valid diff tools and document --tool-help as an option Sebastian Schuberth ` (2 subsequent siblings) 4 siblings, 1 reply; 40+ messages in thread From: Sebastian Schuberth @ 2012-07-23 7:18 UTC (permalink / raw) To: git; +Cc: David Aguilar Also, add a few comments that clarify the meaning of these variables. Signed-off-by: Sebastian Schuberth <sschuberth@gmail.com> --- contrib/completion/git-completion.bash | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash index f2c4894..6b9b79d 100755 --- a/contrib/completion/git-completion.bash +++ b/contrib/completion/git-completion.bash @@ -1325,17 +1325,24 @@ _git_diff () __git_complete_revlist_file } +# Tools that support both merging and diffing. __git_mergetools_common="araxis bc3 diffuse ecmerge emerge gvimdiff kdiff3 meld opendiff p4merge tkdiff vimdiff xxdiff " +# Tools that support diffing. +__git_difftools="$__git_mergetools_common kcompare" + +# Tools that support merging. +__git_mergetools="$__git_mergetools_common tortoisemerge" + _git_difftool () { __git_has_doubledash && return case "$cur" in --tool=*) - __gitcomp "$__git_mergetools_common kompare" "" "${cur##--tool=}" + __gitcomp "$__git_difftools" "" "${cur##--tool=}" return ;; --*) @@ -1623,7 +1630,7 @@ _git_mergetool () { case "$cur" in --tool=*) - __gitcomp "$__git_mergetools_common tortoisemerge" "" "${cur##--tool=}" + __gitcomp "$__git_mergetools" "" "${cur##--tool=}" return ;; --*) -- 1.7.11.msysgit.2 ^ permalink raw reply related [flat|nested] 40+ messages in thread
* Re: [PATCH v2 2/5] Use variables for the lists of tools that support merging / diffing 2012-07-23 7:18 ` [PATCH v2 2/5] Use variables for the lists of tools that support merging / diffing Sebastian Schuberth @ 2012-07-23 16:46 ` Junio C Hamano 2012-07-23 18:30 ` Sebastian Schuberth 0 siblings, 1 reply; 40+ messages in thread From: Junio C Hamano @ 2012-07-23 16:46 UTC (permalink / raw) To: Sebastian Schuberth; +Cc: git, David Aguilar Sebastian Schuberth <sschuberth@gmail.com> writes: > Also, add a few comments that clarify the meaning of these variables. > > Signed-off-by: Sebastian Schuberth <sschuberth@gmail.com> > --- > contrib/completion/git-completion.bash | 11 +++++++++-- > 1 file changed, 9 insertions(+), 2 deletions(-) > > diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash > index f2c4894..6b9b79d 100755 > --- a/contrib/completion/git-completion.bash > +++ b/contrib/completion/git-completion.bash > @@ -1325,17 +1325,24 @@ _git_diff () > __git_complete_revlist_file > } > > +# Tools that support both merging and diffing. > __git_mergetools_common="araxis bc3 diffuse ecmerge emerge gvimdiff > kdiff3 meld opendiff p4merge tkdiff vimdiff xxdiff > " As the set of merge capable tools is not a superset of diff capable tools (tortoise can only merge but not diff), perhaps rename this to __git_diffmerge_tools or something? > +# Tools that support diffing. > +__git_difftools="$__git_mergetools_common kcompare" > + > +# Tools that support merging. > +__git_mergetools="$__git_mergetools_common tortoisemerge" > + This patch makes sense to me, but at the same time makes [PATCH 1/5] a "Meh", methinks. > _git_difftool () > { > __git_has_doubledash && return > > case "$cur" in > --tool=*) > - __gitcomp "$__git_mergetools_common kompare" "" "${cur##--tool=}" > + __gitcomp "$__git_difftools" "" "${cur##--tool=}" > return > ;; > --*) > @@ -1623,7 +1630,7 @@ _git_mergetool () > { > case "$cur" in > --tool=*) > - __gitcomp "$__git_mergetools_common tortoisemerge" "" "${cur##--tool=}" > + __gitcomp "$__git_mergetools" "" "${cur##--tool=}" > return > ;; > --*) ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH v2 2/5] Use variables for the lists of tools that support merging / diffing 2012-07-23 16:46 ` Junio C Hamano @ 2012-07-23 18:30 ` Sebastian Schuberth 2012-07-23 18:32 ` [PATCH 1/4] " Sebastian Schuberth 2012-07-23 18:37 ` [PATCH v2 2/5] " Junio C Hamano 0 siblings, 2 replies; 40+ messages in thread From: Sebastian Schuberth @ 2012-07-23 18:30 UTC (permalink / raw) To: Junio C Hamano; +Cc: git, David Aguilar On 23.07.2012 18:46, Junio C Hamano wrote: >> +# Tools that support both merging and diffing. >> __git_mergetools_common="araxis bc3 diffuse ecmerge emerge gvimdiff >> kdiff3 meld opendiff p4merge tkdiff vimdiff xxdiff >> " > > As the set of merge capable tools is not a superset of diff capable > tools (tortoise can only merge but not diff), perhaps rename this to > __git_diffmerge_tools or something? Makes perfect sense. > This patch makes sense to me, but at the same time makes [PATCH 1/5] > a "Meh", methinks. Yeah, I can see why. So I've renamed __git_mergetools_common to __git_diffmerge_tools and squashed with [PATCH 1/5] to make it less "Meh" as it does not stand on its own. -- Sebastian ^ permalink raw reply [flat|nested] 40+ messages in thread
* [PATCH 1/4] Use variables for the lists of tools that support merging / diffing 2012-07-23 18:30 ` Sebastian Schuberth @ 2012-07-23 18:32 ` Sebastian Schuberth 2012-07-23 18:37 ` [PATCH v2 2/5] " Junio C Hamano 1 sibling, 0 replies; 40+ messages in thread From: Sebastian Schuberth @ 2012-07-23 18:32 UTC (permalink / raw) Cc: Junio C Hamano, git, David Aguilar Also, add a few comments that clarify the meaning of these variables and sort the list of tools alphabetically. Signed-off-by: Sebastian Schuberth <sschuberth@gmail.com> --- contrib/completion/git-completion.bash | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-) diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash index 5be9dee..4a76120 100755 --- a/contrib/completion/git-completion.bash +++ b/contrib/completion/git-completion.bash @@ -1325,17 +1325,24 @@ _git_diff () __git_complete_revlist_file } -__git_mergetools_common="diffuse ecmerge emerge kdiff3 meld opendiff - tkdiff vimdiff gvimdiff xxdiff araxis p4merge bc3 +# Tools that support both merging and diffing. +__git_diffmerge_tools="araxis bc3 diffuse ecmerge emerge gvimdiff + kdiff3 meld opendiff p4merge tkdiff vimdiff xxdiff " +# Tools that support diffing. +__git_difftools="$__git_diffmerge_tools kcompare" + +# Tools that support merging. +__git_mergetools="$__git_diffmerge_tools tortoisemerge" + _git_difftool () { __git_has_doubledash && return case "$cur" in --tool=*) - __gitcomp "$__git_mergetools_common kompare" "" "${cur##--tool=}" + __gitcomp "$__git_difftools" "" "${cur##--tool=}" return ;; --*) @@ -1623,7 +1630,7 @@ _git_mergetool () { case "$cur" in --tool=*) - __gitcomp "$__git_mergetools_common tortoisemerge" "" "${cur##--tool=}" + __gitcomp "$__git_mergetools" "" "${cur##--tool=}" return ;; --*) -- 1.7.11.msysgit.2 ^ permalink raw reply related [flat|nested] 40+ messages in thread
* Re: [PATCH v2 2/5] Use variables for the lists of tools that support merging / diffing 2012-07-23 18:30 ` Sebastian Schuberth 2012-07-23 18:32 ` [PATCH 1/4] " Sebastian Schuberth @ 2012-07-23 18:37 ` Junio C Hamano 2012-07-23 19:03 ` Sebastian Schuberth 1 sibling, 1 reply; 40+ messages in thread From: Junio C Hamano @ 2012-07-23 18:37 UTC (permalink / raw) To: Sebastian Schuberth; +Cc: git, David Aguilar Sebastian Schuberth <sschuberth@gmail.com> writes: >> This patch makes sense to me, but at the same time makes [PATCH 1/5] >> a "Meh", methinks. > > Yeah, I can see why. So I've renamed __git_mergetools_common to > __git_diffmerge_tools and squashed with [PATCH 1/5] to make it > less "Meh" as it does not stand on its own. As you append kcompare or tortoise _after_ the common list, any code that uses the variable cannot assume that the list is sorted, and needs to sort the elements if it wants to give a sorted output, so squashing does not make the Meh-ness go away. By the way, would it make sense to remove these three variables from the completion code, and instead ask "git mergetool --tool-help" when it needs the list of supported tools for the first time? It would be trivial to introduce --tool-list that gives a one tool per line output to both "git difftool" and "git mergetool" and we would remove the risk of separately maintained list drifting away over time. ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH v2 2/5] Use variables for the lists of tools that support merging / diffing 2012-07-23 18:37 ` [PATCH v2 2/5] " Junio C Hamano @ 2012-07-23 19:03 ` Sebastian Schuberth 0 siblings, 0 replies; 40+ messages in thread From: Sebastian Schuberth @ 2012-07-23 19:03 UTC (permalink / raw) To: Junio C Hamano; +Cc: git, David Aguilar On 23.07.2012 20:37, Junio C Hamano wrote: >>> This patch makes sense to me, but at the same time makes [PATCH 1/5] >>> a "Meh", methinks. >> >> Yeah, I can see why. So I've renamed __git_mergetools_common to >> __git_diffmerge_tools and squashed with [PATCH 1/5] to make it >> less "Meh" as it does not stand on its own. > > As you append kcompare or tortoise _after_ the common list, any code > that uses the variable cannot assume that the list is sorted, and > needs to sort the elements if it wants to give a sorted output, so > squashing does not make the Meh-ness go away. Well, that (mostly) sorted listed still helps to find out a little quicker whether a specific tool that can do both merging and diffing is already in the list. At least that's the case for me. > By the way, would it make sense to remove these three variables from > the completion code, and instead ask "git mergetool --tool-help" > when it needs the list of supported tools for the first time? It > would be trivial to introduce --tool-list that gives a one tool per > line output to both "git difftool" and "git mergetool" and we would > remove the risk of separately maintained list drifting away over > time. Sounds like a good idea now that you've added "git mergetool --tool-help". But I'd like to save this for a future exercise to not do too much stuff at the same time. -- Sebastian Schuberth ^ permalink raw reply [flat|nested] 40+ messages in thread
* [PATCH 3/5] Explicitly list all valid diff tools and document --tool-help as an option 2012-07-23 7:14 ` [PATCH v2 0/5] " Sebastian Schuberth 2012-07-23 7:17 ` [PATCH v2 1/5] Sort the list of tools that support both merging and diffing alphabetically Sebastian Schuberth 2012-07-23 7:18 ` [PATCH v2 2/5] Use variables for the lists of tools that support merging / diffing Sebastian Schuberth @ 2012-07-23 7:18 ` Sebastian Schuberth 2012-07-23 16:48 ` Junio C Hamano 2012-07-23 7:19 ` [PATCH v2 4/5] Make sure to use Araxis' "compare" and not e.g. ImageMagick's Sebastian Schuberth 2012-07-23 7:20 ` [PATCH v2 5/5] Add a few more code comments and blank lines in guess_merge_tool Sebastian Schuberth 4 siblings, 1 reply; 40+ messages in thread From: Sebastian Schuberth @ 2012-07-23 7:18 UTC (permalink / raw) To: git; +Cc: David Aguilar Signed-off-by: Sebastian Schuberth <sschuberth@gmail.com> --- Documentation/git-difftool.txt | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/Documentation/git-difftool.txt b/Documentation/git-difftool.txt index 31fc2e3..5dd54f1 100644 --- a/Documentation/git-difftool.txt +++ b/Documentation/git-difftool.txt @@ -36,9 +36,12 @@ OPTIONS -t <tool>:: --tool=<tool>:: - Use the diff tool specified by <tool>. Valid values include - emerge, kompare, meld, and vimdiff. Run `git difftool --tool-help` - for the list of valid <tool> settings. + Use the diff tool specified by <tool>. Valid diff tools are: + araxis, bc3, diffuse, ecmerge, emerge, gvimdiff, kcompare, kdiff3, + meld, opendiff, p4merge, tkdiff, vimdiff and xxdiff. + +--tool-help:: + List the supported and available diff tools. + If a diff tool is not specified, 'git difftool' will use the configuration variable `diff.tool`. If the -- 1.7.11.msysgit.2 ^ permalink raw reply related [flat|nested] 40+ messages in thread
* Re: [PATCH 3/5] Explicitly list all valid diff tools and document --tool-help as an option 2012-07-23 7:18 ` [PATCH 3/5] Explicitly list all valid diff tools and document --tool-help as an option Sebastian Schuberth @ 2012-07-23 16:48 ` Junio C Hamano 2012-07-23 17:21 ` mergetool: support --tool-help option like difftool does Junio C Hamano 0 siblings, 1 reply; 40+ messages in thread From: Junio C Hamano @ 2012-07-23 16:48 UTC (permalink / raw) To: Sebastian Schuberth; +Cc: git, David Aguilar Sebastian Schuberth <sschuberth@gmail.com> writes: > Signed-off-by: Sebastian Schuberth <sschuberth@gmail.com> > --- > Documentation/git-difftool.txt | 9 ++++++--- > 1 file changed, 6 insertions(+), 3 deletions(-) > > diff --git a/Documentation/git-difftool.txt b/Documentation/git-difftool.txt > index 31fc2e3..5dd54f1 100644 > --- a/Documentation/git-difftool.txt > +++ b/Documentation/git-difftool.txt > @@ -36,9 +36,12 @@ OPTIONS > > -t <tool>:: > --tool=<tool>:: > - Use the diff tool specified by <tool>. Valid values include > - emerge, kompare, meld, and vimdiff. Run `git difftool --tool-help` > - for the list of valid <tool> settings. I thought we say "include" because we really do not want to list millions of tools here, so mild NAK on this part. > + Use the diff tool specified by <tool>. Valid diff tools are: > + araxis, bc3, diffuse, ecmerge, emerge, gvimdiff, kcompare, kdiff3, > + meld, opendiff, p4merge, tkdiff, vimdiff and xxdiff. > + > +--tool-help:: > + List the supported and available diff tools. This part is a good addition (but it already is mentioned in the description of --tool above, so it is more or less a "Meh"). > + > If a diff tool is not specified, 'git difftool' > will use the configuration variable `diff.tool`. If the ^ permalink raw reply [flat|nested] 40+ messages in thread
* mergetool: support --tool-help option like difftool does 2012-07-23 16:48 ` Junio C Hamano @ 2012-07-23 17:21 ` Junio C Hamano 2012-07-23 18:56 ` Sebastian Schuberth 2012-08-23 5:33 ` Re*: " Junio C Hamano 0 siblings, 2 replies; 40+ messages in thread From: Junio C Hamano @ 2012-07-23 17:21 UTC (permalink / raw) To: David Aguilar; +Cc: git, Sebastian Schuberth This way we do not have to risk the list of tools go out of sync between the implementation and the documentation. Signed-off-by: Junio C Hamano <gitster@pobox.com> --- Junio C Hamano <gitster@pobox.com> writes: >> +--tool-help:: >> + List the supported and available diff tools. > > This part is a good addition (but it already is mentioned in the > description of --tool above, so it is more or less a "Meh"). I noticed that the main while loop has style violations in its case/esac, but I left it as-is. Reindenting it would be a low hanging fruit janitor patch that would be a separate topic. git-mergetool--lib.sh | 6 +++++- git-mergetool.sh | 42 +++++++++++++++++++++++++++++++++++++++++- 2 files changed, 46 insertions(+), 2 deletions(-) diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh index ed630b2..f730253 100644 --- a/git-mergetool--lib.sh +++ b/git-mergetool--lib.sh @@ -111,7 +111,7 @@ run_merge_tool () { return $status } -guess_merge_tool () { +list_merge_tool_candidates () { if merge_mode then tools="tortoisemerge" @@ -136,6 +136,10 @@ guess_merge_tool () { tools="$tools emerge vimdiff" ;; esac +} + +guess_merge_tool () { + list_merge_tool_candidates echo >&2 "merge tool candidates: $tools" # Loop over each candidate and stop when a valid merge tool is found. diff --git a/git-mergetool.sh b/git-mergetool.sh index a9f23f7..0db0c44 100755 --- a/git-mergetool.sh +++ b/git-mergetool.sh @@ -8,7 +8,7 @@ # at the discretion of Junio C Hamano. # -USAGE='[--tool=tool] [-y|--no-prompt|--prompt] [file to merge] ...' +USAGE='[--tool=tool] [--tool-help] [-y|--no-prompt|--prompt] [file to merge] ...' SUBDIRECTORY_OK=Yes OPTIONS_SPEC= TOOL_MODE=merge @@ -284,11 +284,51 @@ merge_file () { return 0 } +show_tool_help () { + TOOL_MODE=merge + list_merge_tool_candidates + unavailable= available= LF=' +' + for i in $tools + do + merge_tool_path=$(translate_merge_tool_path "$i") + if type "$merge_tool_path" >/dev/null 2>&1 + then + available="$available$i$LF" + else + unavailable="$unavailable$i$LF" + fi + done + if test -n "$available" + then + echo "'git mergetool --tool=<tool>' may be set to one of the following:" + echo "$available" | sort | sed -e 's/^/ /' + else + echo "No suitable tool for 'git mergetool --tool=<tool>' found." + fi + if test -n "$unavailable" + then + echo + echo 'The following tools are valid, but not currently available:' + echo "$unavailable" | sort | sed -e 's/^/ /' + fi + if test -n "$unavailable$available" + then + echo + echo "Some of the tools listed above only work in a windowed" + echo "environment. If run in a terminal-only session, they will fail." + fi + exit 0 +} + prompt=$(git config --bool mergetool.prompt || echo true) while test $# != 0 do case "$1" in + --tool-help) + show_tool_help + ;; -t|--tool*) case "$#,$1" in *,*=*) ^ permalink raw reply related [flat|nested] 40+ messages in thread
* Re: mergetool: support --tool-help option like difftool does 2012-07-23 17:21 ` mergetool: support --tool-help option like difftool does Junio C Hamano @ 2012-07-23 18:56 ` Sebastian Schuberth 2012-07-23 18:58 ` [PATCH] " Sebastian Schuberth 2012-08-23 5:33 ` Re*: " Junio C Hamano 1 sibling, 1 reply; 40+ messages in thread From: Sebastian Schuberth @ 2012-07-23 18:56 UTC (permalink / raw) To: Junio C Hamano; +Cc: David Aguilar, git On 23.07.2012 19:21, Junio C Hamano wrote: > This way we do not have to risk the list of tools go out of sync > between the implementation and the documentation. > > Signed-off-by: Junio C Hamano <gitster@pobox.com> Thanks! I've squashed this with [PATCH v2 5/5] Add a few more code comments and blank lines in guess_merge_tool and parts of [PATCH 3/5] Explicitly list all valid diff tools and document --tool-help as an option and adjusted the docs for git-mergetool accordingly. -- Sebastian Schuberth ^ permalink raw reply [flat|nested] 40+ messages in thread
* [PATCH] mergetool: support --tool-help option like difftool does 2012-07-23 18:56 ` Sebastian Schuberth @ 2012-07-23 18:58 ` Sebastian Schuberth 2012-07-23 19:52 ` Junio C Hamano 0 siblings, 1 reply; 40+ messages in thread From: Sebastian Schuberth @ 2012-07-23 18:58 UTC (permalink / raw) Cc: Junio C Hamano, David Aguilar, git This way we do not have to risk the list of tools go out of sync between the implementation and the documentation. Adjust the documentation accordingly to not explicitly list the tools but refer to --tool-help. Signed-off-by: Junio C Hamano <gitster@pobox.com> Signed-off-by: Sebastian Schuberth <sschuberth@gmail.com> --- Documentation/git-difftool.txt | 7 ++++--- Documentation/git-mergetool.txt | 8 ++++---- git-mergetool--lib.sh | 11 ++++++++++- git-mergetool.sh | 42 ++++++++++++++++++++++++++++++++++++++++- 4 files changed, 59 insertions(+), 9 deletions(-) diff --git a/Documentation/git-difftool.txt b/Documentation/git-difftool.txt index 31fc2e3..0bdfe35 100644 --- a/Documentation/git-difftool.txt +++ b/Documentation/git-difftool.txt @@ -36,9 +36,10 @@ OPTIONS -t <tool>:: --tool=<tool>:: - Use the diff tool specified by <tool>. Valid values include - emerge, kompare, meld, and vimdiff. Run `git difftool --tool-help` - for the list of valid <tool> settings. + Use the diff tool specified by <tool>. + +--tool-help:: + List all supported values for <tool>. + If a diff tool is not specified, 'git difftool' will use the configuration variable `diff.tool`. If the diff --git a/Documentation/git-mergetool.txt b/Documentation/git-mergetool.txt index 2a49de7..99e53b1 100644 --- a/Documentation/git-mergetool.txt +++ b/Documentation/git-mergetool.txt @@ -26,10 +26,10 @@ OPTIONS ------- -t <tool>:: --tool=<tool>:: - Use the merge resolution program specified by <tool>. - Valid merge tools are: - araxis, bc3, diffuse, ecmerge, emerge, gvimdiff, kdiff3, - meld, opendiff, p4merge, tkdiff, tortoisemerge, vimdiff and xxdiff. + Use the merge tool specified by <tool>. + +--tool-help:: + List all supported values for <tool>. + If a merge resolution program is not specified, 'git mergetool' will use the configuration variable `merge.tool`. If the diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh index ed630b2..f0865d4 100644 --- a/git-mergetool--lib.sh +++ b/git-mergetool--lib.sh @@ -111,15 +111,18 @@ run_merge_tool () { return $status } -guess_merge_tool () { +list_merge_tool_candidates () { + # Add tools that can either do merging or diffing, but not both. if merge_mode then tools="tortoisemerge" else tools="kompare" fi + if test -n "$DISPLAY" then + # Prefer GTK-based tools under Gnome. if test -n "$GNOME_DESKTOP_SESSION_ID" then tools="meld opendiff kdiff3 tkdiff xxdiff $tools" @@ -128,6 +131,8 @@ guess_merge_tool () { fi tools="$tools gvimdiff diffuse ecmerge p4merge araxis bc3" fi + + # Prefer vimdiff if vim is the default editor. case "${VISUAL:-$EDITOR}" in *vim*) tools="$tools vimdiff emerge" @@ -136,6 +141,10 @@ guess_merge_tool () { tools="$tools emerge vimdiff" ;; esac +} + +guess_merge_tool () { + list_merge_tool_candidates echo >&2 "merge tool candidates: $tools" # Loop over each candidate and stop when a valid merge tool is found. diff --git a/git-mergetool.sh b/git-mergetool.sh index a9f23f7..0db0c44 100755 --- a/git-mergetool.sh +++ b/git-mergetool.sh @@ -8,7 +8,7 @@ # at the discretion of Junio C Hamano. # -USAGE='[--tool=tool] [-y|--no-prompt|--prompt] [file to merge] ...' +USAGE='[--tool=tool] [--tool-help] [-y|--no-prompt|--prompt] [file to merge] ...' SUBDIRECTORY_OK=Yes OPTIONS_SPEC= TOOL_MODE=merge @@ -284,11 +284,51 @@ merge_file () { return 0 } +show_tool_help () { + TOOL_MODE=merge + list_merge_tool_candidates + unavailable= available= LF=' +' + for i in $tools + do + merge_tool_path=$(translate_merge_tool_path "$i") + if type "$merge_tool_path" >/dev/null 2>&1 + then + available="$available$i$LF" + else + unavailable="$unavailable$i$LF" + fi + done + if test -n "$available" + then + echo "'git mergetool --tool=<tool>' may be set to one of the following:" + echo "$available" | sort | sed -e 's/^/ /' + else + echo "No suitable tool for 'git mergetool --tool=<tool>' found." + fi + if test -n "$unavailable" + then + echo + echo 'The following tools are valid, but not currently available:' + echo "$unavailable" | sort | sed -e 's/^/ /' + fi + if test -n "$unavailable$available" + then + echo + echo "Some of the tools listed above only work in a windowed" + echo "environment. If run in a terminal-only session, they will fail." + fi + exit 0 +} + prompt=$(git config --bool mergetool.prompt || echo true) while test $# != 0 do case "$1" in + --tool-help) + show_tool_help + ;; -t|--tool*) case "$#,$1" in *,*=*) -- 1.7.11.msysgit.2 ^ permalink raw reply related [flat|nested] 40+ messages in thread
* Re: [PATCH] mergetool: support --tool-help option like difftool does 2012-07-23 18:58 ` [PATCH] " Sebastian Schuberth @ 2012-07-23 19:52 ` Junio C Hamano 2012-07-23 20:14 ` Sebastian Schuberth 0 siblings, 1 reply; 40+ messages in thread From: Junio C Hamano @ 2012-07-23 19:52 UTC (permalink / raw) To: Sebastian Schuberth; +Cc: David Aguilar, git Sebastian Schuberth <sschuberth@gmail.com> writes: > This way we do not have to risk the list of tools go out of sync > between the implementation and the documentation. Adjust the documentation > accordingly to not explicitly list the tools but refer to --tool-help. > > Signed-off-by: Junio C Hamano <gitster@pobox.com> > Signed-off-by: Sebastian Schuberth <sschuberth@gmail.com> > --- > Documentation/git-difftool.txt | 7 ++++--- > Documentation/git-mergetool.txt | 8 ++++---- > git-mergetool--lib.sh | 11 ++++++++++- > git-mergetool.sh | 42 ++++++++++++++++++++++++++++++++++++++++- > 4 files changed, 59 insertions(+), 9 deletions(-) > > diff --git a/Documentation/git-difftool.txt b/Documentation/git-difftool.txt > index 31fc2e3..0bdfe35 100644 > --- a/Documentation/git-difftool.txt > +++ b/Documentation/git-difftool.txt > @@ -36,9 +36,10 @@ OPTIONS > > -t <tool>:: > --tool=<tool>:: > - Use the diff tool specified by <tool>. Valid values include > - emerge, kompare, meld, and vimdiff. Run `git difftool --tool-help` > - for the list of valid <tool> settings. > + Use the diff tool specified by <tool>. I do not see how it is an improvement to drop the most common ones. People sometimes read documentation without having an access to shell to run "cmd --tool-help", and a list of handful of well known ones would serve as a good hint to let the reader know the kind of commands the front-end is capable of spawning, which in turn help such a reader to imagine how the command is used to judge if it is something the reader wants to use. > + > +--tool-help:: > + List all supported values for <tool>. > + > If a diff tool is not specified, 'git difftool' > will use the configuration variable `diff.tool`. If the > diff --git a/Documentation/git-mergetool.txt b/Documentation/git-mergetool.txt > index 2a49de7..99e53b1 100644 > --- a/Documentation/git-mergetool.txt > +++ b/Documentation/git-mergetool.txt > @@ -26,10 +26,10 @@ OPTIONS > ------- > -t <tool>:: > --tool=<tool>:: > - Use the merge resolution program specified by <tool>. > - Valid merge tools are: > - araxis, bc3, diffuse, ecmerge, emerge, gvimdiff, kdiff3, > - meld, opendiff, p4merge, tkdiff, tortoisemerge, vimdiff and xxdiff. > + Use the merge tool specified by <tool>. Likewise. Dropping araxis, bc3, etc. to trim the list to 4-5 items that people would know what they are would make sense, though. The purpose of the list is not to tell if the reader's favorite tool is supported or not---it is to hint the flavor of what kind of things the command spawned would be capable of. > + > +--tool-help:: > + List all supported values for <tool>. > + > If a merge resolution program is not specified, 'git mergetool' > will use the configuration variable `merge.tool`. If the > diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh > index ed630b2..f0865d4 100644 > --- a/git-mergetool--lib.sh > +++ b/git-mergetool--lib.sh > @@ -111,15 +111,18 @@ run_merge_tool () { > return $status > } > > -guess_merge_tool () { > +list_merge_tool_candidates () { > + # Add tools that can either do merging or diffing, but not both. > if merge_mode > then > tools="tortoisemerge" > else > tools="kompare" > fi > + > if test -n "$DISPLAY" > then > + # Prefer GTK-based tools under Gnome. > if test -n "$GNOME_DESKTOP_SESSION_ID" > then > tools="meld opendiff kdiff3 tkdiff xxdiff $tools" > @@ -128,6 +131,8 @@ guess_merge_tool () { > fi > tools="$tools gvimdiff diffuse ecmerge p4merge araxis bc3" > fi > + > + # Prefer vimdiff if vim is the default editor. > case "${VISUAL:-$EDITOR}" in > *vim*) > tools="$tools vimdiff emerge" > @@ -136,6 +141,10 @@ guess_merge_tool () { > tools="$tools emerge vimdiff" > ;; > esac > +} > + > +guess_merge_tool () { > + list_merge_tool_candidates > echo >&2 "merge tool candidates: $tools" > > # Loop over each candidate and stop when a valid merge tool is found. > diff --git a/git-mergetool.sh b/git-mergetool.sh > index a9f23f7..0db0c44 100755 > --- a/git-mergetool.sh > +++ b/git-mergetool.sh > @@ -8,7 +8,7 @@ > # at the discretion of Junio C Hamano. > # > > -USAGE='[--tool=tool] [-y|--no-prompt|--prompt] [file to merge] ...' > +USAGE='[--tool=tool] [--tool-help] [-y|--no-prompt|--prompt] [file to merge] ...' > SUBDIRECTORY_OK=Yes > OPTIONS_SPEC= > TOOL_MODE=merge > @@ -284,11 +284,51 @@ merge_file () { > return 0 > } > > +show_tool_help () { > + TOOL_MODE=merge > + list_merge_tool_candidates > + unavailable= available= LF=' > +' > + for i in $tools > + do > + merge_tool_path=$(translate_merge_tool_path "$i") > + if type "$merge_tool_path" >/dev/null 2>&1 > + then > + available="$available$i$LF" > + else > + unavailable="$unavailable$i$LF" > + fi > + done > + if test -n "$available" > + then > + echo "'git mergetool --tool=<tool>' may be set to one of the following:" > + echo "$available" | sort | sed -e 's/^/ /' > + else > + echo "No suitable tool for 'git mergetool --tool=<tool>' found." > + fi > + if test -n "$unavailable" > + then > + echo > + echo 'The following tools are valid, but not currently available:' > + echo "$unavailable" | sort | sed -e 's/^/ /' > + fi > + if test -n "$unavailable$available" > + then > + echo > + echo "Some of the tools listed above only work in a windowed" > + echo "environment. If run in a terminal-only session, they will fail." > + fi > + exit 0 > +} > + > prompt=$(git config --bool mergetool.prompt || echo true) > > while test $# != 0 > do > case "$1" in > + --tool-help) > + show_tool_help > + ;; > -t|--tool*) > case "$#,$1" in > *,*=*) ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH] mergetool: support --tool-help option like difftool does 2012-07-23 19:52 ` Junio C Hamano @ 2012-07-23 20:14 ` Sebastian Schuberth 2012-07-23 20:44 ` David Aguilar 0 siblings, 1 reply; 40+ messages in thread From: Sebastian Schuberth @ 2012-07-23 20:14 UTC (permalink / raw) To: Junio C Hamano; +Cc: David Aguilar, git On Mon, Jul 23, 2012 at 9:52 PM, Junio C Hamano <gitster@pobox.com> wrote: >> -t <tool>:: >> --tool=<tool>:: >> - Use the diff tool specified by <tool>. Valid values include >> - emerge, kompare, meld, and vimdiff. Run `git difftool --tool-help` >> - for the list of valid <tool> settings. >> + Use the diff tool specified by <tool>. > > I do not see how it is an improvement to drop the most common ones. > People sometimes read documentation without having an access to > shell to run "cmd --tool-help", and a list of handful of well known > ones would serve as a good hint to let the reader know the kind of > commands the front-end is capable of spawning, which in turn help > such a reader to imagine how the command is used to judge if it is > something the reader wants to use. I don't agree. What "most common ones" are depends on your platform and is sort of subjective. So it should be either all or non here. Your argument about people not having shell access is a valid one, but still that would mean to list all tools in my opinion. And listing all tools again thwarts our goal to reduce the number of places where new merge / diff tools need to be added. For people adding new merge / diff tools it is just clearer what places need to be modified if there are no places that list an arbitrary subset of tools. -- Sebastian Schuberth ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH] mergetool: support --tool-help option like difftool does 2012-07-23 20:14 ` Sebastian Schuberth @ 2012-07-23 20:44 ` David Aguilar 2012-07-23 21:16 ` Junio C Hamano 0 siblings, 1 reply; 40+ messages in thread From: David Aguilar @ 2012-07-23 20:44 UTC (permalink / raw) To: Sebastian Schuberth; +Cc: Junio C Hamano, git, Tim Henigan On Mon, Jul 23, 2012 at 1:14 PM, Sebastian Schuberth <sschuberth@gmail.com> wrote: > On Mon, Jul 23, 2012 at 9:52 PM, Junio C Hamano <gitster@pobox.com> wrote: > >>> -t <tool>:: >>> --tool=<tool>:: >>> - Use the diff tool specified by <tool>. Valid values include >>> - emerge, kompare, meld, and vimdiff. Run `git difftool --tool-help` >>> - for the list of valid <tool> settings. >>> + Use the diff tool specified by <tool>. >> >> I do not see how it is an improvement to drop the most common ones. >> People sometimes read documentation without having an access to >> shell to run "cmd --tool-help", and a list of handful of well known >> ones would serve as a good hint to let the reader know the kind of >> commands the front-end is capable of spawning, which in turn help >> such a reader to imagine how the command is used to judge if it is >> something the reader wants to use. > > I don't agree. What "most common ones" are depends on your platform > and is sort of subjective. So it should be either all or non here. Let's please leave this section as-is. This part of the documentation has had a fair amount of churn. Specifically, it would get touched every time a new tool was added. The point of bf73fc212a159210398b6d46ed5e9101c650e7db was to change it *one last time* into something that is helpful, but not a substitute for the real list output by --tool-help. If any changes are done then it should be to make git-mergetool.txt match the advice given in git-difftool.txt. > Your argument about people not having shell access is a valid one, but > still that would mean to list all tools in my opinion. And listing all > tools again thwarts our goal to reduce the number of places where new > merge / diff tools need to be added. For people adding new merge / > diff tools it is just clearer what places need to be modified if there > are no places that list an arbitrary subset of tools. Yes, indeed, it is arbitrary. It does have some merit, though--it is also a good compromise between unhelpful (listing nothing) and painful to maintain (listing everything). -- David ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH] mergetool: support --tool-help option like difftool does 2012-07-23 20:44 ` David Aguilar @ 2012-07-23 21:16 ` Junio C Hamano 2012-07-23 21:27 ` Sebastian Schuberth 0 siblings, 1 reply; 40+ messages in thread From: Junio C Hamano @ 2012-07-23 21:16 UTC (permalink / raw) To: David Aguilar; +Cc: Sebastian Schuberth, git, Tim Henigan David Aguilar <davvid@gmail.com> writes: > On Mon, Jul 23, 2012 at 1:14 PM, Sebastian Schuberth > <sschuberth@gmail.com> wrote: >> On Mon, Jul 23, 2012 at 9:52 PM, Junio C Hamano <gitster@pobox.com> wrote: >> >>>> -t <tool>:: >>>> --tool=<tool>:: >>>> - Use the diff tool specified by <tool>. Valid values include >>>> - emerge, kompare, meld, and vimdiff. Run `git difftool --tool-help` >>>> - for the list of valid <tool> settings. >>>> + Use the diff tool specified by <tool>. >>> >>> I do not see how it is an improvement to drop the most common ones. >>> People sometimes read documentation without having an access to >>> shell to run "cmd --tool-help", and a list of handful of well known >>> ones would serve as a good hint to let the reader know the kind of >>> commands the front-end is capable of spawning, which in turn help >>> such a reader to imagine how the command is used to judge if it is >>> something the reader wants to use. >> >> I don't agree. What "most common ones" are depends on your platform >> and is sort of subjective. So it should be either all or non here. > > Let's please leave this section as-is. Yes. There are only five or six classes of environment that matter in the real world for the purpose of giving "well known" examples: Windows, MacOS X, Gnome, KDE and Linux terminal. By picking a representative one from each and listing them, the end result would have at least one that people from various platforms have _heard of_ and can guess what they do. The "most common" is secondary, and "well known" is the keyword. "Can guess what they do" is the point of the phrase "Valid values _include_". Even if you are hard-core Emacs user, it is likely that you've heard of vimdiff---and in that case, including vimdiff would be enough. Likewise for showing kompare to Gnome users. Unlike POSIXy folks, where IRIX or Solaris users are likely to have heard of Gnome tools even if they do not use the environment on their platforms, Windows users tend to be isolated bunch, so it would not hurt to include at least one well-known Windows-only tool in the list. > Yes, indeed, it is arbitrary. It does have some merit, though--it is > also a good compromise between unhelpful (listing nothing) and painful > to maintain (listing everything). Here is a v2, with documentation updates. -- >8 -- Subject: [PATCH] mergetool: support --tool-help option like difftool does This way we do not have to risk the list of tools going out of sync between the implementation and the documentation. In the same spirit as bf73fc2 (difftool: print list of valid tools with '--tool-help', 2012-03-29), trim the list of merge backends in the documentation. We do not want to have a complete of valid tools there; we only want a list to help people guess what kind of things the tools that can be specified there, and refer them to --tool-help for a complete list. Signed-off-by: Junio C Hamano <gitster@pobox.com> --- Documentation/git-mergetool.txt | 6 +++--- git-mergetool--lib.sh | 6 +++++- git-mergetool.sh | 42 ++++++++++++++++++++++++++++++++++++++++- 3 files changed, 49 insertions(+), 5 deletions(-) diff --git a/Documentation/git-mergetool.txt b/Documentation/git-mergetool.txt index 2a49de7..d1e08d2 100644 --- a/Documentation/git-mergetool.txt +++ b/Documentation/git-mergetool.txt @@ -27,9 +27,9 @@ OPTIONS -t <tool>:: --tool=<tool>:: Use the merge resolution program specified by <tool>. - Valid merge tools are: - araxis, bc3, diffuse, ecmerge, emerge, gvimdiff, kdiff3, - meld, opendiff, p4merge, tkdiff, tortoisemerge, vimdiff and xxdiff. + Valid values include emerge, gvimdiff, kdiff3, + meld, vimdiff, and tortoisemerge. Run `git mergetool --tool-help` + for the list of valid <tool> settings. + If a merge resolution program is not specified, 'git mergetool' will use the configuration variable `merge.tool`. If the diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh index ed630b2..f730253 100644 --- a/git-mergetool--lib.sh +++ b/git-mergetool--lib.sh @@ -111,7 +111,7 @@ run_merge_tool () { return $status } -guess_merge_tool () { +list_merge_tool_candidates () { if merge_mode then tools="tortoisemerge" @@ -136,6 +136,10 @@ guess_merge_tool () { tools="$tools emerge vimdiff" ;; esac +} + +guess_merge_tool () { + list_merge_tool_candidates echo >&2 "merge tool candidates: $tools" # Loop over each candidate and stop when a valid merge tool is found. diff --git a/git-mergetool.sh b/git-mergetool.sh index a9f23f7..0db0c44 100755 --- a/git-mergetool.sh +++ b/git-mergetool.sh @@ -8,7 +8,7 @@ # at the discretion of Junio C Hamano. # -USAGE='[--tool=tool] [-y|--no-prompt|--prompt] [file to merge] ...' +USAGE='[--tool=tool] [--tool-help] [-y|--no-prompt|--prompt] [file to merge] ...' SUBDIRECTORY_OK=Yes OPTIONS_SPEC= TOOL_MODE=merge @@ -284,11 +284,51 @@ merge_file () { return 0 } +show_tool_help () { + TOOL_MODE=merge + list_merge_tool_candidates + unavailable= available= LF=' +' + for i in $tools + do + merge_tool_path=$(translate_merge_tool_path "$i") + if type "$merge_tool_path" >/dev/null 2>&1 + then + available="$available$i$LF" + else + unavailable="$unavailable$i$LF" + fi + done + if test -n "$available" + then + echo "'git mergetool --tool=<tool>' may be set to one of the following:" + echo "$available" | sort | sed -e 's/^/ /' + else + echo "No suitable tool for 'git mergetool --tool=<tool>' found." + fi + if test -n "$unavailable" + then + echo + echo 'The following tools are valid, but not currently available:' + echo "$unavailable" | sort | sed -e 's/^/ /' + fi + if test -n "$unavailable$available" + then + echo + echo "Some of the tools listed above only work in a windowed" + echo "environment. If run in a terminal-only session, they will fail." + fi + exit 0 +} + prompt=$(git config --bool mergetool.prompt || echo true) while test $# != 0 do case "$1" in + --tool-help) + show_tool_help + ;; -t|--tool*) case "$#,$1" in *,*=*) -- 1.7.11.3.339.gbdbf398 ^ permalink raw reply related [flat|nested] 40+ messages in thread
* Re: [PATCH] mergetool: support --tool-help option like difftool does 2012-07-23 21:16 ` Junio C Hamano @ 2012-07-23 21:27 ` Sebastian Schuberth 2012-07-23 22:31 ` Junio C Hamano 0 siblings, 1 reply; 40+ messages in thread From: Sebastian Schuberth @ 2012-07-23 21:27 UTC (permalink / raw) To: Junio C Hamano; +Cc: David Aguilar, git, Tim Henigan On Mon, Jul 23, 2012 at 11:16 PM, Junio C Hamano <gitster@pobox.com> wrote: > There are only five or six classes of environment that matter in the > real world for the purpose of giving "well known" examples: Windows, > MacOS X, Gnome, KDE and Linux terminal. By picking a representative > one from each and listing them, the end result would have at least > one that people from various platforms have _heard of_ and can guess > what they do. The "most common" is secondary, and "well known" is I completely agree with this. So we should take the chance and add a Windows representative to the list of difftools, no? > Unlike POSIXy folks, where IRIX or Solaris users are likely to have > heard of Gnome tools even if they do not use the environment on > their platforms, Windows users tend to be isolated bunch, so it > would not hurt to include at least one well-known Windows-only tool > in the list. Heh. I believe POSIX folks are no less isolated. (How many Windows-only tools would you recognize by name?) They're just isolated in a bigger world ;-) > Here is a v2, with documentation updates. This drops the explicit mention of --tool-help as an option in the documentation compared to my patch. Do you want to keep --tool-help being mentioned inline as part of the --tool option documentation only? -- Sebastian Schuberth ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH] mergetool: support --tool-help option like difftool does 2012-07-23 21:27 ` Sebastian Schuberth @ 2012-07-23 22:31 ` Junio C Hamano 0 siblings, 0 replies; 40+ messages in thread From: Junio C Hamano @ 2012-07-23 22:31 UTC (permalink / raw) To: Sebastian Schuberth; +Cc: David Aguilar, git, Tim Henigan Sebastian Schuberth <sschuberth@gmail.com> writes: > On Mon, Jul 23, 2012 at 11:16 PM, Junio C Hamano <gitster@pobox.com> wrote: > >> There are only five or six classes of environment that matter in the >> real world for the purpose of giving "well known" examples: Windows, >> MacOS X, Gnome, KDE and Linux terminal. By picking a representative >> one from each and listing them, the end result would have at least >> one that people from various platforms have _heard of_ and can guess >> what they do. The "most common" is secondary, and "well known" is > > I completely agree with this. So we should take the chance and add a > Windows representative to the list of difftools, no? I do not care very deeply either way. I am more interested in seeing --tool-list options supported by both so that we can get rid of hardcoded list from the completion script. > This drops the explicit mention of --tool-help as an option in the > documentation compared to my patch. Do you want to keep --tool-help > being mentioned inline as part of the --tool option documentation > only? While I do not think having one would hurt that much, I do not think a separate entry would add much value either, so I chose not to clutter the documentation further. The "--tool=<tool>" entry is enough hint to draw eyes of readers who want to find out what kind of backends are supported, and --tool-help is mentioned there quite prominently. ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re*: mergetool: support --tool-help option like difftool does 2012-07-23 17:21 ` mergetool: support --tool-help option like difftool does Junio C Hamano 2012-07-23 18:56 ` Sebastian Schuberth @ 2012-08-23 5:33 ` Junio C Hamano 2012-08-23 7:39 ` David Aguilar 1 sibling, 1 reply; 40+ messages in thread From: Junio C Hamano @ 2012-08-23 5:33 UTC (permalink / raw) To: git; +Cc: David Aguilar, Sebastian Schuberth Junio C Hamano <gitster@pobox.com> writes: > This way we do not have to risk the list of tools go out of sync > between the implementation and the documentation. > > Signed-off-by: Junio C Hamano <gitster@pobox.com> > --- > Junio C Hamano <gitster@pobox.com> writes: > >>> +--tool-help:: >>> + List the supported and available diff tools. >> >> This part is a good addition (but it already is mentioned in the >> description of --tool above, so it is more or less a "Meh"). > > I noticed that the main while loop has style violations in its > case/esac, but I left it as-is. Reindenting it would be a low > hanging fruit janitor patch that would be a separate topic. After hinting a low-hanging-fruit that would be an easy way for new people to dip their toes, I saw no takers for one month, so I ended up doing it myself. -- >8 -- Subject: mergetool: style fixes This script is one of the sizeable ones that tempted people to copy its "neibouring style" in their new code, but was littered with styles incompatible with our style guide. - use one tab, not four spaces, per indent level; - long lines can be wrapped after '|', '&&', or '||' for readability. - structures like "if .. then .. else .. fi", "while .. do .. done" are split into lines in such a way that does not require unnecessary semicolon. - case, esac and case-arms align at the same column. Signed-off-by: Junio C Hamano <gitster@pobox.com> --- * The change would be easier to see if the reader runs these command before and after applying this patch: $ git diff -w git-mergetool.sh $ git grep -e '^[\t]* ' -e '; *then' -e '; do' git-mergetool.sh git-mergetool.sh | 581 +++++++++++++++++++++++++++++-------------------------- 1 file changed, 308 insertions(+), 273 deletions(-) diff --git i/git-mergetool.sh w/git-mergetool.sh index 0db0c44..c50e18a 100755 --- i/git-mergetool.sh +++ w/git-mergetool.sh @@ -18,270 +18,301 @@ require_work_tree # Returns true if the mode reflects a symlink is_symlink () { - test "$1" = 120000 + test "$1" = 120000 } is_submodule () { - test "$1" = 160000 + test "$1" = 160000 } local_present () { - test -n "$local_mode" + test -n "$local_mode" } remote_present () { - test -n "$remote_mode" + test -n "$remote_mode" } base_present () { - test -n "$base_mode" + test -n "$base_mode" } cleanup_temp_files () { - if test "$1" = --save-backup ; then - rm -rf -- "$MERGED.orig" - test -e "$BACKUP" && mv -- "$BACKUP" "$MERGED.orig" - rm -f -- "$LOCAL" "$REMOTE" "$BASE" - else - rm -f -- "$LOCAL" "$REMOTE" "$BASE" "$BACKUP" - fi + if test "$1" = --save-backup + then + rm -rf -- "$MERGED.orig" + test -e "$BACKUP" && mv -- "$BACKUP" "$MERGED.orig" + rm -f -- "$LOCAL" "$REMOTE" "$BASE" + else + rm -f -- "$LOCAL" "$REMOTE" "$BASE" "$BACKUP" + fi } describe_file () { - mode="$1" - branch="$2" - file="$3" - - printf " {%s}: " "$branch" - if test -z "$mode"; then - echo "deleted" - elif is_symlink "$mode" ; then - echo "a symbolic link -> '$(cat "$file")'" - elif is_submodule "$mode" ; then - echo "submodule commit $file" - else - if base_present; then - echo "modified file" + mode="$1" + branch="$2" + file="$3" + + printf " {%s}: " "$branch" + if test -z "$mode" + then + echo "deleted" + elif is_symlink "$mode" + then + echo "a symbolic link -> '$(cat "$file")'" + elif is_submodule "$mode" + then + echo "submodule commit $file" + elif base_present + then + echo "modified file" else - echo "created file" + echo "created file" fi - fi } - resolve_symlink_merge () { - while true; do - printf "Use (l)ocal or (r)emote, or (a)bort? " - read ans || return 1 - case "$ans" in - [lL]*) - git checkout-index -f --stage=2 -- "$MERGED" - git add -- "$MERGED" - cleanup_temp_files --save-backup - return 0 - ;; - [rR]*) - git checkout-index -f --stage=3 -- "$MERGED" - git add -- "$MERGED" - cleanup_temp_files --save-backup - return 0 - ;; - [aA]*) - return 1 - ;; - esac + while true + do + printf "Use (l)ocal or (r)emote, or (a)bort? " + read ans || return 1 + case "$ans" in + [lL]*) + git checkout-index -f --stage=2 -- "$MERGED" + git add -- "$MERGED" + cleanup_temp_files --save-backup + return 0 + ;; + [rR]*) + git checkout-index -f --stage=3 -- "$MERGED" + git add -- "$MERGED" + cleanup_temp_files --save-backup + return 0 + ;; + [aA]*) + return 1 + ;; + esac done } resolve_deleted_merge () { - while true; do - if base_present; then - printf "Use (m)odified or (d)eleted file, or (a)bort? " - else - printf "Use (c)reated or (d)eleted file, or (a)bort? " - fi - read ans || return 1 - case "$ans" in - [mMcC]*) - git add -- "$MERGED" - cleanup_temp_files --save-backup - return 0 - ;; - [dD]*) - git rm -- "$MERGED" > /dev/null - cleanup_temp_files - return 0 - ;; - [aA]*) - return 1 - ;; - esac + while true + do + if base_present + then + printf "Use (m)odified or (d)eleted file, or (a)bort? " + else + printf "Use (c)reated or (d)eleted file, or (a)bort? " + fi + read ans || return 1 + case "$ans" in + [mMcC]*) + git add -- "$MERGED" + cleanup_temp_files --save-backup + return 0 + ;; + [dD]*) + git rm -- "$MERGED" > /dev/null + cleanup_temp_files + return 0 + ;; + [aA]*) + return 1 + ;; + esac done } resolve_submodule_merge () { - while true; do - printf "Use (l)ocal or (r)emote, or (a)bort? " - read ans || return 1 - case "$ans" in - [lL]*) - if ! local_present; then - if test -n "$(git ls-tree HEAD -- "$MERGED")"; then - # Local isn't present, but it's a subdirectory - git ls-tree --full-name -r HEAD -- "$MERGED" | git update-index --index-info || exit $? - else - test -e "$MERGED" && mv -- "$MERGED" "$BACKUP" - git update-index --force-remove "$MERGED" + while true + do + printf "Use (l)ocal or (r)emote, or (a)bort? " + read ans || return 1 + case "$ans" in + [lL]*) + if ! local_present + then + if test -n "$(git ls-tree HEAD -- "$MERGED")" + then + # Local isn't present, but it's a subdirectory + git ls-tree --full-name -r HEAD -- "$MERGED" | + git update-index --index-info || exit $? + else + test -e "$MERGED" && mv -- "$MERGED" "$BACKUP" + git update-index --force-remove "$MERGED" + cleanup_temp_files --save-backup + fi + elif is_submodule "$local_mode" + then + stage_submodule "$MERGED" "$local_sha1" + else + git checkout-index -f --stage=2 -- "$MERGED" + git add -- "$MERGED" + fi + return 0 + ;; + [rR]*) + if ! remote_present + then + if test -n "$(git ls-tree MERGE_HEAD -- "$MERGED")" + then + # Remote isn't present, but it's a subdirectory + git ls-tree --full-name -r MERGE_HEAD -- "$MERGED" | + git update-index --index-info || exit $? + else + test -e "$MERGED" && mv -- "$MERGED" "$BACKUP" + git update-index --force-remove "$MERGED" + fi + elif is_submodule "$remote_mode" + then + ! is_submodule "$local_mode" && + test -e "$MERGED" && + mv -- "$MERGED" "$BACKUP" + stage_submodule "$MERGED" "$remote_sha1" + else + test -e "$MERGED" && mv -- "$MERGED" "$BACKUP" + git checkout-index -f --stage=3 -- "$MERGED" + git add -- "$MERGED" + fi cleanup_temp_files --save-backup - fi - elif is_submodule "$local_mode"; then - stage_submodule "$MERGED" "$local_sha1" - else - git checkout-index -f --stage=2 -- "$MERGED" - git add -- "$MERGED" - fi - return 0 - ;; - [rR]*) - if ! remote_present; then - if test -n "$(git ls-tree MERGE_HEAD -- "$MERGED")"; then - # Remote isn't present, but it's a subdirectory - git ls-tree --full-name -r MERGE_HEAD -- "$MERGED" | git update-index --index-info || exit $? - else - test -e "$MERGED" && mv -- "$MERGED" "$BACKUP" - git update-index --force-remove "$MERGED" - fi - elif is_submodule "$remote_mode"; then - ! is_submodule "$local_mode" && test -e "$MERGED" && mv -- "$MERGED" "$BACKUP" - stage_submodule "$MERGED" "$remote_sha1" - else - test -e "$MERGED" && mv -- "$MERGED" "$BACKUP" - git checkout-index -f --stage=3 -- "$MERGED" - git add -- "$MERGED" - fi - cleanup_temp_files --save-backup - return 0 - ;; - [aA]*) - return 1 - ;; - esac + return 0 + ;; + [aA]*) + return 1 + ;; + esac done } stage_submodule () { - path="$1" - submodule_sha1="$2" - mkdir -p "$path" || die "fatal: unable to create directory for module at $path" - # Find $path relative to work tree - work_tree_root=$(cd_to_toplevel && pwd) - work_rel_path=$(cd "$path" && GIT_WORK_TREE="${work_tree_root}" git rev-parse --show-prefix) - test -n "$work_rel_path" || die "fatal: unable to get path of module $path relative to work tree" - git update-index --add --replace --cacheinfo 160000 "$submodule_sha1" "${work_rel_path%/}" || die + path="$1" + submodule_sha1="$2" + mkdir -p "$path" || + die "fatal: unable to create directory for module at $path" + # Find $path relative to work tree + work_tree_root=$(cd_to_toplevel && pwd) + work_rel_path=$(cd "$path" && + GIT_WORK_TREE="${work_tree_root}" git rev-parse --show-prefix + ) + test -n "$work_rel_path" || + die "fatal: unable to get path of module $path relative to work tree" + git update-index --add --replace --cacheinfo 160000 "$submodule_sha1" "${work_rel_path%/}" || die } checkout_staged_file () { - tmpfile=$(expr \ - "$(git checkout-index --temp --stage="$1" "$2" 2>/dev/null)" \ - : '\([^ ]*\) ') - - if test $? -eq 0 -a -n "$tmpfile" ; then - mv -- "$(git rev-parse --show-cdup)$tmpfile" "$3" - else - >"$3" - fi + tmpfile=$(expr \ + "$(git checkout-index --temp --stage="$1" "$2" 2>/dev/null)" \ + : '\([^ ]*\) ') + + if test $? -eq 0 -a -n "$tmpfile" + then + mv -- "$(git rev-parse --show-cdup)$tmpfile" "$3" + else + >"$3" + fi } merge_file () { - MERGED="$1" + MERGED="$1" - f=$(git ls-files -u -- "$MERGED") - if test -z "$f" ; then - if test ! -f "$MERGED" ; then - echo "$MERGED: file not found" - else - echo "$MERGED: file does not need merging" + f=$(git ls-files -u -- "$MERGED") + if test -z "$f" + then + if test ! -f "$MERGED" + then + echo "$MERGED: file not found" + else + echo "$MERGED: file does not need merging" + fi + return 1 fi - return 1 - fi - - ext="$$$(expr "$MERGED" : '.*\(\.[^/]*\)$')" - BACKUP="./$MERGED.BACKUP.$ext" - LOCAL="./$MERGED.LOCAL.$ext" - REMOTE="./$MERGED.REMOTE.$ext" - BASE="./$MERGED.BASE.$ext" - - base_mode=$(git ls-files -u -- "$MERGED" | awk '{if ($3==1) print $1;}') - local_mode=$(git ls-files -u -- "$MERGED" | awk '{if ($3==2) print $1;}') - remote_mode=$(git ls-files -u -- "$MERGED" | awk '{if ($3==3) print $1;}') - - if is_submodule "$local_mode" || is_submodule "$remote_mode"; then - echo "Submodule merge conflict for '$MERGED':" - local_sha1=$(git ls-files -u -- "$MERGED" | awk '{if ($3==2) print $2;}') - remote_sha1=$(git ls-files -u -- "$MERGED" | awk '{if ($3==3) print $2;}') - describe_file "$local_mode" "local" "$local_sha1" - describe_file "$remote_mode" "remote" "$remote_sha1" - resolve_submodule_merge - return - fi - - mv -- "$MERGED" "$BACKUP" - cp -- "$BACKUP" "$MERGED" - - checkout_staged_file 1 "$MERGED" "$BASE" - checkout_staged_file 2 "$MERGED" "$LOCAL" - checkout_staged_file 3 "$MERGED" "$REMOTE" - - if test -z "$local_mode" -o -z "$remote_mode"; then - echo "Deleted merge conflict for '$MERGED':" - describe_file "$local_mode" "local" "$LOCAL" - describe_file "$remote_mode" "remote" "$REMOTE" - resolve_deleted_merge - return - fi - if is_symlink "$local_mode" || is_symlink "$remote_mode"; then - echo "Symbolic link merge conflict for '$MERGED':" + ext="$$$(expr "$MERGED" : '.*\(\.[^/]*\)$')" + BACKUP="./$MERGED.BACKUP.$ext" + LOCAL="./$MERGED.LOCAL.$ext" + REMOTE="./$MERGED.REMOTE.$ext" + BASE="./$MERGED.BASE.$ext" + + base_mode=$(git ls-files -u -- "$MERGED" | awk '{if ($3==1) print $1;}') + local_mode=$(git ls-files -u -- "$MERGED" | awk '{if ($3==2) print $1;}') + remote_mode=$(git ls-files -u -- "$MERGED" | awk '{if ($3==3) print $1;}') + + if is_submodule "$local_mode" || is_submodule "$remote_mode" + then + echo "Submodule merge conflict for '$MERGED':" + local_sha1=$(git ls-files -u -- "$MERGED" | awk '{if ($3==2) print $2;}') + remote_sha1=$(git ls-files -u -- "$MERGED" | awk '{if ($3==3) print $2;}') + describe_file "$local_mode" "local" "$local_sha1" + describe_file "$remote_mode" "remote" "$remote_sha1" + resolve_submodule_merge + return + fi + + mv -- "$MERGED" "$BACKUP" + cp -- "$BACKUP" "$MERGED" + + checkout_staged_file 1 "$MERGED" "$BASE" + checkout_staged_file 2 "$MERGED" "$LOCAL" + checkout_staged_file 3 "$MERGED" "$REMOTE" + + if test -z "$local_mode" -o -z "$remote_mode" + then + echo "Deleted merge conflict for '$MERGED':" + describe_file "$local_mode" "local" "$LOCAL" + describe_file "$remote_mode" "remote" "$REMOTE" + resolve_deleted_merge + return + fi + + if is_symlink "$local_mode" || is_symlink "$remote_mode" + then + echo "Symbolic link merge conflict for '$MERGED':" + describe_file "$local_mode" "local" "$LOCAL" + describe_file "$remote_mode" "remote" "$REMOTE" + resolve_symlink_merge + return + fi + + echo "Normal merge conflict for '$MERGED':" describe_file "$local_mode" "local" "$LOCAL" describe_file "$remote_mode" "remote" "$REMOTE" - resolve_symlink_merge - return - fi - - echo "Normal merge conflict for '$MERGED':" - describe_file "$local_mode" "local" "$LOCAL" - describe_file "$remote_mode" "remote" "$REMOTE" - if "$prompt" = true; then - printf "Hit return to start merge resolution tool (%s): " "$merge_tool" - read ans || return 1 - fi - - if base_present; then - present=true - else - present=false - fi - - if ! run_merge_tool "$merge_tool" "$present"; then - echo "merge of $MERGED failed" 1>&2 - mv -- "$BACKUP" "$MERGED" - - if test "$merge_keep_temporaries" = "false"; then - cleanup_temp_files + if "$prompt" = true + then + printf "Hit return to start merge resolution tool (%s): " "$merge_tool" + read ans || return 1 fi - return 1 - fi + if base_present + then + present=true + else + present=false + fi + + if ! run_merge_tool "$merge_tool" "$present" + then + echo "merge of $MERGED failed" 1>&2 + mv -- "$BACKUP" "$MERGED" + + if test "$merge_keep_temporaries" = "false" + then + cleanup_temp_files + fi - if test "$merge_keep_backup" = "true"; then - mv -- "$BACKUP" "$MERGED.orig" - else - rm -- "$BACKUP" - fi + return 1 + fi - git add -- "$MERGED" - cleanup_temp_files - return 0 + if test "$merge_keep_backup" = "true" + then + mv -- "$BACKUP" "$MERGED.orig" + else + rm -- "$BACKUP" + fi + + git add -- "$MERGED" + cleanup_temp_files + return 0 } show_tool_help () { @@ -325,61 +356,61 @@ prompt=$(git config --bool mergetool.prompt || echo true) while test $# != 0 do - case "$1" in + case "$1" in --tool-help) show_tool_help ;; -t|--tool*) - case "$#,$1" in + case "$#,$1" in *,*=*) - merge_tool=$(expr "z$1" : 'z-[^=]*=\(.*\)') - ;; + merge_tool=$(expr "z$1" : 'z-[^=]*=\(.*\)') + ;; 1,*) - usage ;; + usage ;; *) - merge_tool="$2" - shift ;; - esac - ;; + merge_tool="$2" + shift ;; + esac + ;; -y|--no-prompt) - prompt=false - ;; + prompt=false + ;; --prompt) - prompt=true - ;; + prompt=true + ;; --) - shift - break - ;; + shift + break + ;; -*) - usage - ;; - *) - break - ;; - esac - shift -done - -prompt_after_failed_merge() { - while true; do - printf "Continue merging other unresolved paths (y/n) ? " - read ans || return 1 - case "$ans" in - - [yY]*) - return 0 + usage ;; - - [nN]*) - return 1 + *) + break ;; esac - done + shift +done + +prompt_after_failed_merge () { + while true + do + printf "Continue merging other unresolved paths (y/n) ? " + read ans || return 1 + case "$ans" in + [yY]*) + return 0 + ;; + [nN]*) + return 1 + ;; + esac + done } -if test -z "$merge_tool"; then - merge_tool=$(get_merge_tool "$merge_tool") || exit +if test -z "$merge_tool" +then + merge_tool=$(get_merge_tool "$merge_tool") || exit fi merge_keep_backup="$(git config --bool mergetool.keepBackup || echo true)" merge_keep_temporaries="$(git config --bool mergetool.keepTemporaries || echo false)" @@ -388,22 +419,24 @@ last_status=0 rollup_status=0 files= -if test $# -eq 0 ; then - cd_to_toplevel +if test $# -eq 0 +then + cd_to_toplevel - if test -e "$GIT_DIR/MERGE_RR" - then - files=$(git rerere remaining) - else - files=$(git ls-files -u | sed -e 's/^[^ ]* //' | sort -u) - fi + if test -e "$GIT_DIR/MERGE_RR" + then + files=$(git rerere remaining) + else + files=$(git ls-files -u | sed -e 's/^[^ ]* //' | sort -u) + fi else - files=$(git ls-files -u -- "$@" | sed -e 's/^[^ ]* //' | sort -u) + files=$(git ls-files -u -- "$@" | sed -e 's/^[^ ]* //' | sort -u) fi -if test -z "$files" ; then - echo "No files need merging" - exit 0 +if test -z "$files" +then + echo "No files need merging" + exit 0 fi printf "Merging:\n" @@ -413,15 +446,17 @@ IFS=' ' for i in $files do - if test $last_status -ne 0; then - prompt_after_failed_merge || exit 1 - fi - printf "\n" - merge_file "$i" - last_status=$? - if test $last_status -ne 0; then - rollup_status=1 - fi + if test $last_status -ne 0 + then + prompt_after_failed_merge || exit 1 + fi + printf "\n" + merge_file "$i" + last_status=$? + if test $last_status -ne 0 + then + rollup_status=1 + fi done exit $rollup_status ^ permalink raw reply related [flat|nested] 40+ messages in thread
* Re: Re*: mergetool: support --tool-help option like difftool does 2012-08-23 5:33 ` Re*: " Junio C Hamano @ 2012-08-23 7:39 ` David Aguilar 2012-08-23 17:39 ` Junio C Hamano 0 siblings, 1 reply; 40+ messages in thread From: David Aguilar @ 2012-08-23 7:39 UTC (permalink / raw) To: Junio C Hamano; +Cc: git, Sebastian Schuberth On Wed, Aug 22, 2012 at 10:33 PM, Junio C Hamano <junio@pobox.com> wrote: > Junio C Hamano <gitster@pobox.com> writes: > >> This way we do not have to risk the list of tools go out of sync >> between the implementation and the documentation. >> >> Signed-off-by: Junio C Hamano <gitster@pobox.com> >> --- >> Junio C Hamano <gitster@pobox.com> writes: >> >>>> +--tool-help:: >>>> + List the supported and available diff tools. >>> >>> This part is a good addition (but it already is mentioned in the >>> description of --tool above, so it is more or less a "Meh"). >> >> I noticed that the main while loop has style violations in its >> case/esac, but I left it as-is. Reindenting it would be a low >> hanging fruit janitor patch that would be a separate topic. > > After hinting a low-hanging-fruit that would be an easy way for new > people to dip their toes, I saw no takers for one month, so I ended > up doing it myself. My bad, I didn't catch this and should have, especially so because I had started on style fixing mergetool last week but ditched it; I assumed it would be unwanted. I will read more carefully. Just please don't tell Charles about it ;-) (I wanted to do this a long ago but at the time we didn't have the style guide for shell, and the time was perhaps not right...) I am of course in favor of this patch. While on the mergetool topic... Would the ability to resolve the various merge situations using the command-line be a wanted addition? This would let a submodule or deleted/modified encountering user do something like: $ git mergetool --theirs -- submodule ...and not have to remember the various git commands that it runs. This could touch just the parts of mergetool that require stdin, but probably should work for everything. It seems like exposing the guts of some of these mergetool functions via command-line flags could be helpful, but I'm not sure if that's overloading mergetool with too many features. What do you think? -- David ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: Re*: mergetool: support --tool-help option like difftool does 2012-08-23 7:39 ` David Aguilar @ 2012-08-23 17:39 ` Junio C Hamano 2012-08-24 8:31 ` David Aguilar 0 siblings, 1 reply; 40+ messages in thread From: Junio C Hamano @ 2012-08-23 17:39 UTC (permalink / raw) To: David Aguilar; +Cc: git, Sebastian Schuberth, Jens Lehmann, Heiko Voigt David Aguilar <davvid@gmail.com> writes: >> After hinting a low-hanging-fruit that would be an easy way for new >> people to dip their toes, I saw no takers for one month, so I ended >> up doing it myself. > > My bad,... Not yours. These hints I drop from time to time are meant for eager new people who want to dip their toes to the development (we don't do "assignments" but these are the closest that we have); you no longer quite qualify as "new" ;-) > While on the mergetool topic... Now we are not talking about "dip their toes" low hanging fruit anymore ;-) > Would the ability to resolve the various merge situations using > the command-line be a wanted addition? > > This would let a submodule or deleted/modified encountering > user do something like: > > $ git mergetool --theirs -- submodule > > ...and not have to remember the various git commands that it runs. Does it have to run various git commands? For a normal path, all it needs to do is "git checkout --theirs $path", no? What does it mean to resolve a conflicted merge of a gitlink to take "theirs"? We obviously would want to point the resolved gitlink at the submodule commit their side wants in the resulting index but what, if any, should we do to the submodule itself? Stepping back a bit, if there is no conflict, and as a result of a clean merge (this applies to the case where you check out another branch that has different commit at the submodule path), if gitlink changed to point at a different commit in the submodule, what should happen? If you start from a clean working tree, with your gitlink pointing at the commit that matches HEAD in the submodule, and if the working tree of the submodule does not have any local modification, it may be ideal to check out the new commit in the submodule (are there cases where "git checkout other_branch" in the superproject does not want to touch the submodule working tree?). There are cases where it is not possible; checking out the new commit in the submodule working tree may not succeed due to local modifications. But is that kind of complication limited to the merge resolution case? Isn't it shared with normal "switching branches" case as well? If so, it could be that your "git mergetool --theirs -- submodule" is working around a wrong problem, and the right solution may be to make "git checkout --theirs -- $path" to always do an appropriate thing regardless of what kind of object $path is, no? ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: Re*: mergetool: support --tool-help option like difftool does 2012-08-23 17:39 ` Junio C Hamano @ 2012-08-24 8:31 ` David Aguilar 2012-08-26 18:38 ` Jens Lehmann 0 siblings, 1 reply; 40+ messages in thread From: David Aguilar @ 2012-08-24 8:31 UTC (permalink / raw) To: Junio C Hamano; +Cc: git, Sebastian Schuberth, Jens Lehmann, Heiko Voigt On Thu, Aug 23, 2012 at 10:39 AM, Junio C Hamano <gitster@pobox.com> wrote: > David Aguilar <davvid@gmail.com> writes: >> Would the ability to resolve the various merge situations using >> the command-line be a wanted addition? >> >> This would let a submodule or deleted/modified encountering >> user do something like: >> >> $ git mergetool --theirs -- submodule >> >> ...and not have to remember the various git commands that it runs. > > Does it have to run various git commands? For a normal path, all it > needs to do is "git checkout --theirs $path", no? > > What does it mean to resolve a conflicted merge of a gitlink to take > "theirs"? We obviously would want to point the resolved gitlink at > the submodule commit their side wants in the resulting index but what, > if any, should we do to the submodule itself? > > Stepping back a bit, if there is no conflict, and as a result of a > clean merge (this applies to the case where you check out another > branch that has different commit at the submodule path), if gitlink > changed to point at a different commit in the submodule, what should > happen? > > If you start from a clean working tree, with your gitlink pointing > at the commit that matches HEAD in the submodule, and if the working > tree of the submodule does not have any local modification, it may > be ideal to check out the new commit in the submodule (are there > cases where "git checkout other_branch" in the superproject does not > want to touch the submodule working tree?). > > There are cases where it is not possible; checking out the new > commit in the submodule working tree may not succeed due to local > modifications. But is that kind of complication limited to the > merge resolution case? Isn't it shared with normal "switching > branches" case as well? > > If so, it could be that your "git mergetool --theirs -- submodule" > is working around a wrong problem, and the right solution may be to > make "git checkout --theirs -- $path" to always do an appropriate > thing regardless of what kind of object $path is, no? True. Admittedly, I'm not a heavy submodule user myself so I tried crafting the directory vs submodule conflict to see what the usability is like. checkout --theirs and --ours could learn a few tricks. When trying to choose the directory in that situation I had to had to "git rm --cached" the submodule path so that git would recognize that there was no longer a conflict. That makes sense to me because I was following along by reading the mergetool code, but I don't think most users would know to "git rm" a path which they intend to keep. Afterwards, the .git file is left behind, which could cause problems elsewhere since we really don't want a .git file in that situation. I'm not even sure what to do about the .gitmodules file either. Here's the script I was using to setup the merge conflict in case anyone wants to get a feel for the usability around this edge case. Pass --submodule if you want the remote side to have a submodule. By default, the local side has a submodule and the remote side of the merge brings along a directory. That said, this really isn't an issue, per say. I first poked at it because I noticed that mergetool still needed stdin for some things. It's certainly an edge case, and perhaps this just shows that mergetool really is the right porcelain for the job when a user runs into these types of conflicts (the stdin thing really isn't an issue). #!/bin/sh if test "$1" = "--submodule" then first=with-directory second=with-submodule echo "local will be a directory, remote will be a submodule" else first=with-submodule second=with-directory echo "local will be a submodule, remote will be a directory" fi repo=submodule-conflict-$$ && echo "creating $repo" && mkdir "$repo" && cd "$repo" && git init && git commit --allow-empty -m'initial' && git checkout -b with-directory master && mkdir the-conflict && touch the-conflict/path && git add the-conflict/path && git commit -m'added path into the-conflict' && git checkout -b with-submodule master && git submodule add ./ the-conflict && git commit -m'added submodule as the-conflict' && git checkout -b tmp-merge master && git merge $first && git merge $second -- David ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: Re*: mergetool: support --tool-help option like difftool does 2012-08-24 8:31 ` David Aguilar @ 2012-08-26 18:38 ` Jens Lehmann 0 siblings, 0 replies; 40+ messages in thread From: Jens Lehmann @ 2012-08-26 18:38 UTC (permalink / raw) To: David Aguilar; +Cc: Junio C Hamano, git, Sebastian Schuberth, Heiko Voigt Am 24.08.2012 10:31, schrieb David Aguilar: > On Thu, Aug 23, 2012 at 10:39 AM, Junio C Hamano <gitster@pobox.com> wrote: >> David Aguilar <davvid@gmail.com> writes: >>> Would the ability to resolve the various merge situations using >>> the command-line be a wanted addition? >>> >>> This would let a submodule or deleted/modified encountering >>> user do something like: >>> >>> $ git mergetool --theirs -- submodule >>> >>> ...and not have to remember the various git commands that it runs. >> >> Does it have to run various git commands? For a normal path, all it >> needs to do is "git checkout --theirs $path", no? >> >> What does it mean to resolve a conflicted merge of a gitlink to take >> "theirs"? We obviously would want to point the resolved gitlink at >> the submodule commit their side wants in the resulting index but what, >> if any, should we do to the submodule itself? >> >> Stepping back a bit, if there is no conflict, and as a result of a >> clean merge (this applies to the case where you check out another >> branch that has different commit at the submodule path), if gitlink >> changed to point at a different commit in the submodule, what should >> happen? >> >> If you start from a clean working tree, with your gitlink pointing >> at the commit that matches HEAD in the submodule, and if the working >> tree of the submodule does not have any local modification, it may >> be ideal to check out the new commit in the submodule (are there >> cases where "git checkout other_branch" in the superproject does not >> want to touch the submodule working tree?). >> >> There are cases where it is not possible; checking out the new >> commit in the submodule working tree may not succeed due to local >> modifications. But is that kind of complication limited to the >> merge resolution case? Isn't it shared with normal "switching >> branches" case as well? >> >> If so, it could be that your "git mergetool --theirs -- submodule" >> is working around a wrong problem, and the right solution may be to >> make "git checkout --theirs -- $path" to always do an appropriate >> thing regardless of what kind of object $path is, no? > > True. I agree. > Admittedly, I'm not a heavy submodule user myself so I > tried crafting the directory vs submodule conflict to see > what the usability is like. > > checkout --theirs and --ours could learn a few tricks. Me thinks that after I successfully taught checkout to properly recurse into submodule work trees too it should know all those tricks. In my current version it can handle directory/submodule conversions in both directions, this should be sufficient to make "checkout --theirs/--ours" work properly. Note to self: add tests for that. > When trying to choose the directory in that situation > I had to had to "git rm --cached" the submodule path > so that git would recognize that there was no longer a conflict. > > That makes sense to me because I was following along by > reading the mergetool code, but I don't think most users > would know to "git rm" a path which they intend to keep. True. But a submodule recursing checkout would do the right thing here too. > Afterwards, the .git file is left behind, which could cause > problems elsewhere since we really don't want a .git file > in that situation. Hmm, either you remove all the files tracked in the submodule together with the gitfile or you'll possibly have former submodule files lying around there too. Recursive checkout will do all that for you. > I'm not even sure what to do about the > .gitmodules file either. Maybe we should issue a warning when the .gitmodules file is not consistent with the [non]existence of a submodule in the work tree? > That said, this really isn't an issue, per say. > I first poked at it because I noticed that mergetool > still needed stdin for some things. > > It's certainly an edge case, and perhaps this just shows > that mergetool really is the right porcelain for the job > when a user runs into these types of conflicts > (the stdin thing really isn't an issue). It looks to me as if the submodule/directory conflict can be handled by a recursive checkout without having to add something to mergetool. ^ permalink raw reply [flat|nested] 40+ messages in thread
* [PATCH v2 4/5] Make sure to use Araxis' "compare" and not e.g. ImageMagick's 2012-07-23 7:14 ` [PATCH v2 0/5] " Sebastian Schuberth ` (2 preceding siblings ...) 2012-07-23 7:18 ` [PATCH 3/5] Explicitly list all valid diff tools and document --tool-help as an option Sebastian Schuberth @ 2012-07-23 7:19 ` Sebastian Schuberth 2012-07-23 16:50 ` Junio C Hamano 2012-07-23 7:20 ` [PATCH v2 5/5] Add a few more code comments and blank lines in guess_merge_tool Sebastian Schuberth 4 siblings, 1 reply; 40+ messages in thread From: Sebastian Schuberth @ 2012-07-23 7:19 UTC (permalink / raw) To: git; +Cc: David Aguilar Signed-off-by: Sebastian Schuberth <sschuberth@gmail.com> --- mergetools/araxis | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/mergetools/araxis b/mergetools/araxis index 64f97c5..aeba1b9 100644 --- a/mergetools/araxis +++ b/mergetools/araxis @@ -16,5 +16,11 @@ merge_cmd () { } translate_merge_tool_path() { - echo compare + # Make sure to use Araxis' "compare" and not e.g. ImageMagick's. + if ls "$(dirname "$(which compare)")"/Araxis* >/dev/null 2>&1 + then + echo compare + else + echo "$1" + fi } -- 1.7.11.msysgit.2 ^ permalink raw reply related [flat|nested] 40+ messages in thread
* Re: [PATCH v2 4/5] Make sure to use Araxis' "compare" and not e.g. ImageMagick's 2012-07-23 7:19 ` [PATCH v2 4/5] Make sure to use Araxis' "compare" and not e.g. ImageMagick's Sebastian Schuberth @ 2012-07-23 16:50 ` Junio C Hamano 2012-07-23 19:37 ` [PATCH] " Sebastian Schuberth 0 siblings, 1 reply; 40+ messages in thread From: Junio C Hamano @ 2012-07-23 16:50 UTC (permalink / raw) To: Sebastian Schuberth; +Cc: git, David Aguilar Sebastian Schuberth <sschuberth@gmail.com> writes: > Signed-off-by: Sebastian Schuberth <sschuberth@gmail.com> > --- > mergetools/araxis | 8 +++++++- > 1 file changed, 7 insertions(+), 1 deletion(-) > > diff --git a/mergetools/araxis b/mergetools/araxis > index 64f97c5..aeba1b9 100644 > --- a/mergetools/araxis > +++ b/mergetools/araxis > @@ -16,5 +16,11 @@ merge_cmd () { > } > > translate_merge_tool_path() { > - echo compare > + # Make sure to use Araxis' "compare" and not e.g. ImageMagick's. > + if ls "$(dirname "$(which compare)")"/Araxis* >/dev/null 2>&1 Use of "which" in scripts that are meant to be portable is a no-no. Some platforms would say "compare is /usr/local/bin/compare" instead of saying "/usr/local/bin/compare". > + then > + echo compare > + else > + echo "$1" > + fi > } ^ permalink raw reply [flat|nested] 40+ messages in thread
* [PATCH] Make sure to use Araxis' "compare" and not e.g. ImageMagick's 2012-07-23 16:50 ` Junio C Hamano @ 2012-07-23 19:37 ` Sebastian Schuberth 2012-07-23 20:47 ` Junio C Hamano 0 siblings, 1 reply; 40+ messages in thread From: Sebastian Schuberth @ 2012-07-23 19:37 UTC (permalink / raw) To: Junio C Hamano; +Cc: git, David Aguilar Signed-off-by: Sebastian Schuberth <sschuberth@gmail.com> --- mergetools/araxis | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/mergetools/araxis b/mergetools/araxis index 64f97c5..f8899f8 100644 --- a/mergetools/araxis +++ b/mergetools/araxis @@ -16,5 +16,12 @@ merge_cmd () { } translate_merge_tool_path() { - echo compare + # Only accept "compare" in a path that contains "araxis" to not + # accidently use e.g. ImageMagick's "compare". + if type compare | grep -i araxis >/dev/null 2>&1 + then + echo compare + else + echo "$1" + fi } -- 1.7.11.msysgit.2 ^ permalink raw reply related [flat|nested] 40+ messages in thread
* Re: [PATCH] Make sure to use Araxis' "compare" and not e.g. ImageMagick's 2012-07-23 19:37 ` [PATCH] " Sebastian Schuberth @ 2012-07-23 20:47 ` Junio C Hamano 2012-07-23 21:09 ` Sebastian Schuberth 0 siblings, 1 reply; 40+ messages in thread From: Junio C Hamano @ 2012-07-23 20:47 UTC (permalink / raw) To: Sebastian Schuberth; +Cc: git, David Aguilar Sebastian Schuberth <sschuberth@gmail.com> writes: > Signed-off-by: Sebastian Schuberth <sschuberth@gmail.com> > --- > mergetools/araxis | 9 ++++++++- > 1 file changed, 8 insertions(+), 1 deletion(-) > > diff --git a/mergetools/araxis b/mergetools/araxis > index 64f97c5..f8899f8 100644 > --- a/mergetools/araxis > +++ b/mergetools/araxis > @@ -16,5 +16,12 @@ merge_cmd () { > } > > translate_merge_tool_path() { > - echo compare > + # Only accept "compare" in a path that contains "araxis" to not > + # accidently use e.g. ImageMagick's "compare". > + if type compare | grep -i araxis >/dev/null 2>&1 > + then > + echo compare > + else > + echo "$1" > + fi > } I do not use araxis, and I do not know how rigid its installation procedure is to prevent the end user from naming a path that does not have the string in it. For example, when the user tells it to install in "/home/ss/bin", if it installs its "compare" program in "/home/ss/bin/araxis/compare" without allowing the "/araxis/" part to be stripped away, the above heuristics is sufficiently safe. Otherwise, it is not. It is unclear from your proposed commit log message what assurance do we have that it is installed under such a path and why the heuristics the patch implements is the sane way forward. Please convince me. ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH] Make sure to use Araxis' "compare" and not e.g. ImageMagick's 2012-07-23 20:47 ` Junio C Hamano @ 2012-07-23 21:09 ` Sebastian Schuberth 2012-07-23 21:24 ` Junio C Hamano ` (2 more replies) 0 siblings, 3 replies; 40+ messages in thread From: Sebastian Schuberth @ 2012-07-23 21:09 UTC (permalink / raw) To: Junio C Hamano; +Cc: git, David Aguilar On Mon, Jul 23, 2012 at 10:47 PM, Junio C Hamano <gitster@pobox.com> wrote: > For example, when the user tells it to install in "/home/ss/bin", if > it installs its "compare" program in "/home/ss/bin/araxis/compare" > without allowing the "/araxis/" part to be stripped away, the above > heuristics is sufficiently safe. Otherwise, it is not. To the best of my knowledge, Araxis does not enforce any naming convention for the path it gets installed in. That means the user may indeed install the program in a path that does not contain "araxis". I was aware of this when writing the patch, but should have probably made it more clear in the commit message. > It is unclear from your proposed commit log message what assurance > do we have that it is installed under such a path and why the > heuristics the patch implements is the sane way forward. We have no such assurance. That's why you correctly call it a heuristics after all: it may fail. Personally, I've valued the gain of the patch to not list araxis as an available diff tool by "git difftool --tool-help" when in fact just ImageMagick is in PATH higher than the loss to support araxis installations that are in a path not containung "araxis" but are in PATH. Please feel free to ignore the patch if you feel the heuristics is not sufficiently safe. I'm currently unable to come up with a safer solution while maintaining portability, i.e. not use "which" or doing rather laborious string parsing on the output of "type". -- Sebastian Schuberth ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH] Make sure to use Araxis' "compare" and not e.g. ImageMagick's 2012-07-23 21:09 ` Sebastian Schuberth @ 2012-07-23 21:24 ` Junio C Hamano 2012-07-23 21:31 ` Sebastian Schuberth 2012-07-23 21:34 ` David Aguilar 2012-07-23 21:26 ` Andreas Schwab 2012-07-23 22:22 ` Junio C Hamano 2 siblings, 2 replies; 40+ messages in thread From: Junio C Hamano @ 2012-07-23 21:24 UTC (permalink / raw) To: Sebastian Schuberth; +Cc: git, David Aguilar Sebastian Schuberth <sschuberth@gmail.com> writes: > We have no such assurance. That's why you correctly call it a > heuristics after all ImageMagick "compare" takes "--version" and says something like this to its standard output: $ compare --version Version: ImageMagick 6.6.0-4 2012-05-02 Q16 http://www.imagemagick.org Copyright: Copyright (C) 1999-2010 ImageMagick Studio LLC Does Araxis compare take "--version" and behave in a way that is cheaply controllable? If it opens a GUI window and pops up a dialog that says "Option not understood", then it is not "controllable", but if it quickly dies with "No such option" sent to the standard error output, or sending its version string to the standard output, then we could use something like: case "$(compare --version 2>/dev/null)" in "Araxis compare version"*) echo compare ;; *) echo "$1" ;; esac instead, and that would be more robust than the path based heuristics. ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH] Make sure to use Araxis' "compare" and not e.g. ImageMagick's 2012-07-23 21:24 ` Junio C Hamano @ 2012-07-23 21:31 ` Sebastian Schuberth 2012-07-23 21:34 ` David Aguilar 1 sibling, 0 replies; 40+ messages in thread From: Sebastian Schuberth @ 2012-07-23 21:31 UTC (permalink / raw) To: Junio C Hamano; +Cc: git, David Aguilar, schwab On Mon, Jul 23, 2012 at 11:24 PM, Junio C Hamano <gitster@pobox.com> wrote: > Does Araxis compare take "--version" and behave in a way that is > cheaply controllable? If it opens a GUI window and pops up a dialog > that says "Option not understood", then it is not "controllable", Araxis opens up a GUI dialog with usage info in that case, so it's not controllable, unfortunately. -- Sebastian Schuberth ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH] Make sure to use Araxis' "compare" and not e.g. ImageMagick's 2012-07-23 21:24 ` Junio C Hamano 2012-07-23 21:31 ` Sebastian Schuberth @ 2012-07-23 21:34 ` David Aguilar 2012-07-23 21:44 ` Sebastian Schuberth 1 sibling, 1 reply; 40+ messages in thread From: David Aguilar @ 2012-07-23 21:34 UTC (permalink / raw) To: Junio C Hamano; +Cc: Sebastian Schuberth, git On Mon, Jul 23, 2012 at 2:24 PM, Junio C Hamano <gitster@pobox.com> wrote: > Sebastian Schuberth <sschuberth@gmail.com> writes: > >> We have no such assurance. That's why you correctly call it a >> heuristics after all > > ImageMagick "compare" takes "--version" and says something like > this to its standard output: > > $ compare --version > Version: ImageMagick 6.6.0-4 2012-05-02 Q16 > http://www.imagemagick.org > Copyright: Copyright (C) 1999-2010 ImageMagick Studio LLC > > Does Araxis compare take "--version" and behave in a way that is > cheaply controllable? If it opens a GUI window and pops up a dialog > that says "Option not understood", then it is not "controllable", > but if it quickly dies with "No such option" sent to the standard > error output, or sending its version string to the standard output, > then we could use something like: > > case "$(compare --version 2>/dev/null)" in > "Araxis compare version"*) > echo compare ;; > *) > echo "$1" ;; > esac > > instead, and that would be more robust than the path based > heuristics. Araxis compare (the one I have) does not accept --version. Also, the GraphicsMagick (ImageMagick fork) compare does not have --version, but it does have "compare version": $ compare version GraphicsMagick 1.3.12 2010-03-08 Q8 http://www.GraphicsMagick.org/ Copyright (C) 2002-2010 GraphicsMagick Group. Additional copyrights and licenses apply to this software. See http://www.GraphicsMagick.org/www/Copyright.html for details. ... If we care to blacklist *Magick compare, then we may be able to call "compare version" and parse the output looking for "Magic". I tested ImageMagick compare and it also understands "compare version". Araxis compare prints nothing when "compare version" is called. Likely because it thinks it has nothing to do. It does the same if I say "compare blahblah", and returns exit status 0. So its output is useless. It seems like the best we can do is specifically blacklist the *Magick "compare" commands so that they do not show up as false positives. And the only way to identify them is to parse their output, since all commands return status 0 for "compare version". Another possibility is to parse the output of "compare" (no args) and grep for "merge". The name "Araxis Merge" is never actually printed, but the help text for "-merge" does appear.... yep.. it's a heuristic. Sebastian, are you testing on Windows? The araxis "compare" I used is OS X. Does "compare version" open a GUI window for you? For me it does not. What about "compare -h", or just "compare" ? -- David ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH] Make sure to use Araxis' "compare" and not e.g. ImageMagick's 2012-07-23 21:34 ` David Aguilar @ 2012-07-23 21:44 ` Sebastian Schuberth 0 siblings, 0 replies; 40+ messages in thread From: Sebastian Schuberth @ 2012-07-23 21:44 UTC (permalink / raw) To: David Aguilar; +Cc: Junio C Hamano, git On Mon, Jul 23, 2012 at 11:34 PM, David Aguilar <davvid@gmail.com> wrote: > Sebastian, are you testing on Windows? The araxis "compare" I used is > OS X. Does "compare version" open a GUI window for you? For me it > does not. > What about "compare -h", or just "compare" ? Yes, I'm testing on Windows, and unfortunately Araxis behaves differently here: - running "compare version" or "compare blah" prints nothing on the console, but the GUI opens saying it is unable to open a file "version" (which it would compare to nothing), - "compare -h" or just "compare" both open up the usage dialog listing all valid command line options (because "-h" is invalid). -- Sebastian Schuberth ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH] Make sure to use Araxis' "compare" and not e.g. ImageMagick's 2012-07-23 21:09 ` Sebastian Schuberth 2012-07-23 21:24 ` Junio C Hamano @ 2012-07-23 21:26 ` Andreas Schwab 2012-07-23 22:22 ` Junio C Hamano 2 siblings, 0 replies; 40+ messages in thread From: Andreas Schwab @ 2012-07-23 21:26 UTC (permalink / raw) To: Sebastian Schuberth; +Cc: Junio C Hamano, git, David Aguilar Sebastian Schuberth <sschuberth@gmail.com> writes: > Please feel free to ignore the patch if you feel the heuristics is not > sufficiently safe. I'm currently unable to come up with a safer > solution while maintaining portability, i.e. not use "which" or doing > rather laborious string parsing on the output of "type". How about looking at "compare --version"? Andreas. -- Andreas Schwab, schwab@linux-m68k.org GPG Key fingerprint = 58CA 54C7 6D53 942B 1756 01D3 44D5 214B 8276 4ED5 "And now for something completely different." ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH] Make sure to use Araxis' "compare" and not e.g. ImageMagick's 2012-07-23 21:09 ` Sebastian Schuberth 2012-07-23 21:24 ` Junio C Hamano 2012-07-23 21:26 ` Andreas Schwab @ 2012-07-23 22:22 ` Junio C Hamano 2012-07-23 22:39 ` Sebastian Schuberth 2012-07-23 22:41 ` Junio C Hamano 2 siblings, 2 replies; 40+ messages in thread From: Junio C Hamano @ 2012-07-23 22:22 UTC (permalink / raw) To: Sebastian Schuberth; +Cc: git, David Aguilar Sebastian Schuberth <sschuberth@gmail.com> writes: > Personally, I've valued the gain of > the patch to not list araxis as an available diff tool by "git > difftool --tool-help" when in fact just ImageMagick is in PATH higher > than the loss to support araxis installations that are in a path not > containung "araxis" but are in PATH. I agree that running ImageMagick's compare by accident is one thing we would want to avoid, but once the problem is diagnosed, it is something the user can easily work around by futzing %PATH%, I think. On the other hand, if the user has bought and installed Araxis, but we incorrectly identify it as unusable, the user has wasted good money and there is no easy recourse as far as I can see in your patch. That is why I wanted to see a reasonable assurance. If we limit the problem space by special casing Windows installation (e.g. check "uname -s" or something), would it make the problem easier to solve? Perhaps it is much more likely that the path the program is installed in can be safely identified with a call to "type --path compare" (bash is the only shell shipped in msysgit, isn't it?), and its output is likely to contain "/Program Files/Araxis/" as a substring, or something? ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH] Make sure to use Araxis' "compare" and not e.g. ImageMagick's 2012-07-23 22:22 ` Junio C Hamano @ 2012-07-23 22:39 ` Sebastian Schuberth 2012-07-23 22:41 ` Junio C Hamano 1 sibling, 0 replies; 40+ messages in thread From: Sebastian Schuberth @ 2012-07-23 22:39 UTC (permalink / raw) To: Junio C Hamano; +Cc: git, David Aguilar On Tue, Jul 24, 2012 at 12:22 AM, Junio C Hamano <gitster@pobox.com> wrote: > On the other hand, if the user has bought and installed Araxis, but > we incorrectly identify it as unusable, the user has wasted good > money and there is no easy recourse as far as I can see in your > patch. Agreed. > If we limit the problem space by special casing Windows installation > (e.g. check "uname -s" or something), would it make the problem > easier to solve? Perhaps it is much more likely that the path the > program is installed in can be safely identified with a call to > "type --path compare" (bash is the only shell shipped in msysgit, > isn't it?), and its output is likely to contain "/Program Files/Araxis/" > as a substring, or something? Yes, I could come up with basically a variant of my first version of the patch that is limited to Windows only and uses "type --path" instead of "which", but then again this is too non-generic for my taste. Moreover, as I'm also using Mac OS X, I'd be interested in a solution that works there, too. I don't see a good (read generic and concise) solution to the issue. I think we should just drop my patch and not waste all of our time on it any more. -- Sebastian Schuberth ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH] Make sure to use Araxis' "compare" and not e.g. ImageMagick's 2012-07-23 22:22 ` Junio C Hamano 2012-07-23 22:39 ` Sebastian Schuberth @ 2012-07-23 22:41 ` Junio C Hamano 2012-07-23 23:09 ` Sebastian Schuberth 1 sibling, 1 reply; 40+ messages in thread From: Junio C Hamano @ 2012-07-23 22:41 UTC (permalink / raw) To: Sebastian Schuberth; +Cc: git, David Aguilar Junio C Hamano <gitster@pobox.com> writes: > If we limit the problem space by special casing Windows installation > (e.g. check "uname -s" or something), would it make the problem > easier to solve? Perhaps it is much more likely that the path the > program is installed in can be safely identified with a call to > "type --path compare" (bash is the only shell shipped in msysgit, > isn't it?). E.g. something along the lines of your original patch. I do not know what other commands are typically installed in the same directory as "compare", so it is likely you need to fix the name of the file to let us positively identify "compare" is from the Araxis suite. mergetools/araxis | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-) diff --git a/mergetools/araxis b/mergetools/araxis index 64f97c5..1180a32 100644 --- a/mergetools/araxis +++ b/mergetools/araxis @@ -16,5 +16,18 @@ merge_cmd () { } translate_merge_tool_path() { - echo compare + case "$BASH_VERSION" in + ??*) + # we can safely use "type --path" + if test -f $(dirname "$(type --path compare)")/AraxisMerge + then + echo compare + else + echo "$1" + fi + ;; + *) + echo compare + ;; + esac } ^ permalink raw reply related [flat|nested] 40+ messages in thread
* Re: [PATCH] Make sure to use Araxis' "compare" and not e.g. ImageMagick's 2012-07-23 22:41 ` Junio C Hamano @ 2012-07-23 23:09 ` Sebastian Schuberth 0 siblings, 0 replies; 40+ messages in thread From: Sebastian Schuberth @ 2012-07-23 23:09 UTC (permalink / raw) To: Junio C Hamano; +Cc: git, David Aguilar On 24.07.2012 00:41, Junio C Hamano wrote: > + if test -f $(dirname "$(type --path compare)")/AraxisMerge We would need additional quotes around the whole path here as the Windows installation path is usually something like "C:\Program Files\Araxis\Araxis Merge" and contains spaces. Moreover, "test -f" requires the ".exe" extension to be explicitly present for the file to test. But I'd rather not do that because the test would be specific to Windows then and e.g. not work on Mac OS X. That's why I'd still like to use ls like in my first patch: mergetools/araxis | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-) diff --git a/mergetools/araxis b/mergetools/araxis index 64f97c5..c406ead 100644 --- a/mergetools/araxis +++ b/mergetools/araxis @@ -16,5 +16,18 @@ merge_cmd () { } translate_merge_tool_path() { - echo compare + case "$BASH_VERSION" in + ??*) + # we can safely use "type --path" + if ls "$(dirname "$(type --path compare)")"/Araxis* >/dev/null 2>&1 + then + echo compare + else + echo "$1" + fi + ;; + *) + echo compare + ;; + esac } -- Sebastian Schuberth ^ permalink raw reply related [flat|nested] 40+ messages in thread
* [PATCH v2 5/5] Add a few more code comments and blank lines in guess_merge_tool 2012-07-23 7:14 ` [PATCH v2 0/5] " Sebastian Schuberth ` (3 preceding siblings ...) 2012-07-23 7:19 ` [PATCH v2 4/5] Make sure to use Araxis' "compare" and not e.g. ImageMagick's Sebastian Schuberth @ 2012-07-23 7:20 ` Sebastian Schuberth 4 siblings, 0 replies; 40+ messages in thread From: Sebastian Schuberth @ 2012-07-23 7:20 UTC (permalink / raw) To: git; +Cc: David Aguilar Signed-off-by: Sebastian Schuberth <sschuberth@gmail.com> --- git-mergetool--lib.sh | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh index ed630b2..ac9a8f0 100644 --- a/git-mergetool--lib.sh +++ b/git-mergetool--lib.sh @@ -112,14 +112,17 @@ run_merge_tool () { } guess_merge_tool () { + # Add tools that can either do merging or diffing, but not both. if merge_mode then tools="tortoisemerge" else tools="kompare" fi + if test -n "$DISPLAY" then + # Prefer GTK-based tools under Gnome. if test -n "$GNOME_DESKTOP_SESSION_ID" then tools="meld opendiff kdiff3 tkdiff xxdiff $tools" @@ -128,6 +131,8 @@ guess_merge_tool () { fi tools="$tools gvimdiff diffuse ecmerge p4merge araxis bc3" fi + + # Prefer vimdiff if vim is the default editor. case "${VISUAL:-$EDITOR}" in *vim*) tools="$tools vimdiff emerge" @@ -136,6 +141,7 @@ guess_merge_tool () { tools="$tools emerge vimdiff" ;; esac + echo >&2 "merge tool candidates: $tools" # Loop over each candidate and stop when a valid merge tool is found. -- 1.7.11.msysgit.2 ^ permalink raw reply related [flat|nested] 40+ messages in thread
end of thread, other threads:[~2012-08-26 18:39 UTC | newest] Thread overview: 40+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2012-07-23 7:10 [PATCH 0/4] Various merge / diff tool related minor clean-ups and improvements Sebastian Schuberth 2012-07-23 7:14 ` [PATCH v2 0/5] " Sebastian Schuberth 2012-07-23 7:17 ` [PATCH v2 1/5] Sort the list of tools that support both merging and diffing alphabetically Sebastian Schuberth 2012-07-23 7:18 ` [PATCH v2 2/5] Use variables for the lists of tools that support merging / diffing Sebastian Schuberth 2012-07-23 16:46 ` Junio C Hamano 2012-07-23 18:30 ` Sebastian Schuberth 2012-07-23 18:32 ` [PATCH 1/4] " Sebastian Schuberth 2012-07-23 18:37 ` [PATCH v2 2/5] " Junio C Hamano 2012-07-23 19:03 ` Sebastian Schuberth 2012-07-23 7:18 ` [PATCH 3/5] Explicitly list all valid diff tools and document --tool-help as an option Sebastian Schuberth 2012-07-23 16:48 ` Junio C Hamano 2012-07-23 17:21 ` mergetool: support --tool-help option like difftool does Junio C Hamano 2012-07-23 18:56 ` Sebastian Schuberth 2012-07-23 18:58 ` [PATCH] " Sebastian Schuberth 2012-07-23 19:52 ` Junio C Hamano 2012-07-23 20:14 ` Sebastian Schuberth 2012-07-23 20:44 ` David Aguilar 2012-07-23 21:16 ` Junio C Hamano 2012-07-23 21:27 ` Sebastian Schuberth 2012-07-23 22:31 ` Junio C Hamano 2012-08-23 5:33 ` Re*: " Junio C Hamano 2012-08-23 7:39 ` David Aguilar 2012-08-23 17:39 ` Junio C Hamano 2012-08-24 8:31 ` David Aguilar 2012-08-26 18:38 ` Jens Lehmann 2012-07-23 7:19 ` [PATCH v2 4/5] Make sure to use Araxis' "compare" and not e.g. ImageMagick's Sebastian Schuberth 2012-07-23 16:50 ` Junio C Hamano 2012-07-23 19:37 ` [PATCH] " Sebastian Schuberth 2012-07-23 20:47 ` Junio C Hamano 2012-07-23 21:09 ` Sebastian Schuberth 2012-07-23 21:24 ` Junio C Hamano 2012-07-23 21:31 ` Sebastian Schuberth 2012-07-23 21:34 ` David Aguilar 2012-07-23 21:44 ` Sebastian Schuberth 2012-07-23 21:26 ` Andreas Schwab 2012-07-23 22:22 ` Junio C Hamano 2012-07-23 22:39 ` Sebastian Schuberth 2012-07-23 22:41 ` Junio C Hamano 2012-07-23 23:09 ` Sebastian Schuberth 2012-07-23 7:20 ` [PATCH v2 5/5] Add a few more code comments and blank lines in guess_merge_tool Sebastian Schuberth
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.