Re: [PATCH 1/8] mergetool: use tabs consistently
- From
Charles Bailey <charles@hashpling.org>
- Date
- Mar 30, 2009, 21:35 UTC
- Message-ID
- <20090330213530.GA7091@hashpling.org>
- In-Reply-To
- <7vzlf3flim.fsf@gitster.siamese.dyndns.org>
On Mon, Mar 30, 2009 at 01:44:01AM -0700, Junio C Hamano wrote:
Show 13 quoted lines
> 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@hashpling.org> 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/