On Oct 18, 2007, at 10:00 AM, Shawn O. Pearce wrote:
Steffen Prohaska <prohaska@xxxxxx> wrote:
On Oct 18, 2007, at 7:27 AM, Shawn O. Pearce wrote:
...
+ test -n "$merge_tool" || valid_tool "$merge_tool" || {
Wouldn't an empty $merge_tool string be caught above in the
valid_tool function where it falls through and returns 1?
So isn't test -n here redundant?
Sharp eyes, thanks.
Correct is
test -z "$merge_tool" || valid_tool "$merge_tool" || {
No merge tool or a valid merge tool is allowed at this place.
If the tool's already empty there's no need to tell the user
that it will be reset to empty.
I'll send a v4 version.
Thanks but its a little late.
Sorry.
I'm about to push maint/master/next/pu
and this series is in next. While testing it I found another
bug related to init_tool_merge_tool_path() not having $merge_tool
initialized and thus causing it to never select a default tool if
the user didn't have one configured.
I see. Thanks for fixing this.
Here's what I'm actually about to push out:
One remark ...
[...]
if test -z "$merge_tool"; then
merge_tool=`git config merge.tool`
- case "$merge_tool" in
- kdiff3 | tkdiff | xxdiff | meld | opendiff | emerge | vimdiff |
gvimdiff | "")
- ;; # happy
- *)
+ if ! valid_tool "$merge_tool"; then
I think its ok to have an empty tool here ...
echo >&2 "git config option merge.tool set to unknown tool:
$merge_tool"
echo >&2 "Resetting to default..."
... So we should not print this message if the tool is empty.
But an empty tool is not valid. Therefore a check if "$merge_tool"
is not empty should be added to the if.
Steffen
-
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
