On Mon, Mar 30, 2009 at 01:44:01AM -0700, Junio C Hamano wrote:
> Thanks.
>
> Even though this [1/8] is obviously regression free, and I think the
> overall direction in which the series is going is good, I'll wait until I
> hear Acks from Charles Bailey for the parts that involve mergetool. I do
> not use either mergetool nor difftool myself, and going over every single
> line of this series to spot potential regression is beyond my bandwidth
> right now.
>
> I do not think bits only common between mergetool and difftool should be
> called with a very generic name "sh-tools". We didn't call the result of
> a similar refactoring for launching web browser from help and instaweb
> context with such a generic name (it is called git-web--browse).
OK, I've just had a very quick review of the patch series and, in
general, I like it.
I don't much like [1/8] though. I'm all in favour of consistency, but
this patch touches most of the lines in git-mergetool and tries to go
the opposite way to the consistency drive that we were trying to
introduce gradually (i.e. only through lines materially affected by
subsequent patches) in:
commit 0eea345111a9b9fea4dd2841b80bc7d62964e812
Author: Charles Bailey <charles@xxxxxxxxxxxxx>
Date: Thu Nov 13 12:41:13 2008 +0000
Fix some tab/space inconsistencies in git-mergetool.sh
If you'd gone the other way the patch to consistency would only affect
23 lines rather than 347 lines and all bar 3 of these lines you
subsequently remove from git-mergetool.sh in later patches anyway.
[2/8] - looks good.
[3/8] - no mergetool impact.
[4/8] - Hmmm, OK. Even so at this point, I'm getting slightly iffy
feelings about the whole init_merge_tool_path sets a variable needed
by the calling script. I know it's only scripting and not programming,
but it seemed less bad to set (global) variables in sh functions when
they were all in the same sh script.
[5/8] - no mergtool impact.
[6/8] - ditto
[7/8] - OK, here's where my uneasiness about global script variables
vs. parameters really gets going. Why is merge_tool a parameter when
it's setup once and doesn't change in the invocation of a script, yet
base_present is a script global but can vary between sets of paths to
be merged?
I fully appreciate that this is just inheriting the way things are
and that they weren't beautiful before, but it somehow seems even
worse when the variables are set in one script and used from a
function in a separate sourced script. We're definitely setting up a
very strong coupling between the two scripts which will make it harder
to change either in the future.
[8/8] - no mergetool impact here.
On the plus side, I really like the introduction and function of the
run_mergetool function. It's exactly the split that will make
extending mergetool resolves of file vs. symlink vs. directory easier
in the future. I have a similar split in some slow brewing patches
myself.
I think that [1/8] is the only patch that I'd relucatant to ack, as it
seems like unnecessary churn and change of direction. Here's a sample
patch for consistency 'the other way'. As I mentioned before, the
first to hunks are made redundant by your subsequent changes anyway,
so I only counted 3 lines that are currently inconsistent in
git-mergetool as it stands at the moment.
Sample patch fixing consistent whitespace 'the other way'.
---
git-mergetool.sh | 46 +++++++++++++++++++++++-----------------------
1 files changed, 23 insertions(+), 23 deletions(-)
diff --git a/git-mergetool.sh b/git-mergetool.sh
index 87fa88a..1588b5f 100755
--- a/git-mergetool.sh
+++ b/git-mergetool.sh
@@ -344,29 +344,29 @@ valid_custom_tool()
}
valid_tool() {
- case "$1" in
- kdiff3 | tkdiff | xxdiff | meld | opendiff | emerge | vimdiff | gvimdiff | ecmerge)
- ;; # happy
- *)
- if ! valid_custom_tool "$1"; then
- return 1
- fi
- ;;
- esac
+ case "$1" in
+ kdiff3 | tkdiff | xxdiff | meld | opendiff | emerge | vimdiff | gvimdiff | ecmerge)
+ ;; # happy
+ *)
+ if ! valid_custom_tool "$1"; then
+ return 1
+ fi
+ ;;
+ esac
}
init_merge_tool_path() {
- merge_tool_path=`git config mergetool.$1.path`
- if test -z "$merge_tool_path" ; then
- case "$1" in
- emerge)
- merge_tool_path=emacs
- ;;
- *)
- merge_tool_path=$1
- ;;
- esac
- fi
+ merge_tool_path=`git config mergetool.$1.path`
+ if test -z "$merge_tool_path" ; then
+ case "$1" in
+ emerge)
+ merge_tool_path=emacs
+ ;;
+ *)
+ merge_tool_path=$1
+ ;;
+ esac
+ fi
}
prompt_after_failed_merge() {
@@ -389,9 +389,9 @@ prompt_after_failed_merge() {
if test -z "$merge_tool"; then
merge_tool=`git config merge.tool`
if test -n "$merge_tool" && ! valid_tool "$merge_tool"; then
- echo >&2 "git config option merge.tool set to unknown tool: $merge_tool"
- echo >&2 "Resetting to default..."
- unset merge_tool
+ echo >&2 "git config option merge.tool set to unknown tool: $merge_tool"
+ echo >&2 "Resetting to default..."
+ unset merge_tool
fi
fi
--
1.6.2.323.geaf6e
--
Charles Bailey
http://ccgi.hashpling.plus.com/blog/
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
