[PATCH] Copy mergetool "bc3" as "bc4"

[Olivier Croquette]

Beyond compare 4 is out since september 2014. The CLI interface doesn't seem to have changed compared to the version 3.

Signed-off-by: Olivier Croquette <***@free.fr>
---
mergetools/bc4 | 25 +++++++++++++++++++++++++
1 files changed, 25 insertions(+), 0 deletions(-)
create mode 100644 mergetools/bc4

diff --git a/mergetools/bc4 b/mergetools/bc4
new file mode 100644
index 0000000..b6319d2
--- /dev/null
+++ b/mergetools/bc4
@@ -0,0 +1,25 @@
+diff_cmd () {
+ "$merge_tool_path" "$LOCAL" "$REMOTE"
+}
+
+merge_cmd () {
+ touch "$BACKUP"
+ if $base_present
+ then
+ "$merge_tool_path" "$LOCAL" "$REMOTE" "$BASE" \
+ -mergeoutput="$MERGED"
+ else
+ "$merge_tool_path" "$LOCAL" "$REMOTE" \
+ -mergeoutput="$MERGED"
+ fi
+ check_unchanged
+}
+
+translate_merge_tool_path() {
+ if type bcomp >/dev/null 2>/dev/null
+ then
+ echo bcomp
+ else
+ echo bcompare
+ fi
+}

[Junio C Hamano]

Post by Olivier Croquette
Beyond compare 4 is out since september 2014. The CLI interface
doesn't seem to have changed compared to the version 3.

Hmph, if this is identical to mergetools/bc3, why is the patch even
needed? Do we auto-detect something and try to use bc4 which does
not exist and fail, and we must supply a copy as bc4 to prevent it?

It may feel somewhat strange to have to say "mergetool --tool=bc3"
when you know what you have is version 4 and not version 3, but in
that case, I wonder if there are reasons why calling both versions
just "bc" is a bad idea. And assuming that version 5 and later
would keep the same interface, we will not have to worry about that
"I have version 7 why do I have to say 3?" if we can go that route.

Perhaps version 2 and before are unusable as a mergetool backend or
something?

Post by Olivier Croquette
---
mergetools/bc4 | 25 +++++++++++++++++++++++++
1 files changed, 25 insertions(+), 0 deletions(-)
create mode 100644 mergetools/bc4
diff --git a/mergetools/bc4 b/mergetools/bc4
new file mode 100644
index 0000000..b6319d2
--- /dev/null
+++ b/mergetools/bc4
@@ -0,0 +1,25 @@
+diff_cmd () {
+ "$merge_tool_path" "$LOCAL" "$REMOTE"
+}
+
+merge_cmd () {
+ touch "$BACKUP"
+ if $base_present
+ then
+ "$merge_tool_path" "$LOCAL" "$REMOTE" "$BASE" \
+ -mergeoutput="$MERGED"
+ else
+ "$merge_tool_path" "$LOCAL" "$REMOTE" \
+ -mergeoutput="$MERGED"
+ fi
+ check_unchanged
+}
+
+translate_merge_tool_path() {
+ if type bcomp >/dev/null 2>/dev/null
+ then
+ echo bcomp
+ else
+ echo bcompare
+ fi
+}

[Junio C Hamano]

Post by Junio C Hamano

Post by Olivier Croquette
Beyond compare 4 is out since september 2014. The CLI interface
doesn't seem to have changed compared to the version 3.

Hmph, if this is identical to mergetools/bc3, why is the patch even
needed? Do we auto-detect something and try to use bc4 which does
not exist and fail, and we must supply a copy as bc4 to prevent it?
It may feel somewhat strange to have to say "mergetool --tool=bc3"
when you know what you have is version 4 and not version 3, but in
that case, I wonder if there are reasons why calling both versions
just "bc" is a bad idea. And assuming that version 5 and later
would keep the same interface, we will not have to worry about that
"I have version 7 why do I have to say 3?" if we can go that route.
Perhaps version 2 and before are unusable as a mergetool backend or
something?

It seems that ffe6dc08 (mergetool--lib: Add Beyond Compare 3 as a
tool, 2011-02-27) is the first mention of "Beyond Compare" and it
only was interested in version 3 and nothing else.

Perhaps something like this, so that existing users can still use
"bc3" and other people can use "bc" if it bothers them that they
have to say "3" when the backend driver works with both 3 and 4?

---
contrib/completion/git-completion.bash | 2 +-
git-mergetool--lib.sh | 2 +-
mergetools/{bc3 => bc} | 0
mergetools/bc3 | 26 +-------------------------
4 files changed, 3 insertions(+), 27 deletions(-)

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index d548e99..01259cc 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -1203,7 +1203,7 @@ _git_diff ()
}

__git_mergetools_common="diffuse diffmerge ecmerge emerge kdiff3 meld opendiff
- tkdiff vimdiff gvimdiff xxdiff araxis p4merge bc3 codecompare
+ tkdiff vimdiff gvimdiff xxdiff araxis p4merge bc codecompare
"

_git_difftool ()
diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh
index c45a020..1d8a26d 100644
--- a/git-mergetool--lib.sh
+++ b/git-mergetool--lib.sh
@@ -250,7 +250,7 @@ list_merge_tool_candidates () {
tools="opendiff kdiff3 tkdiff xxdiff meld $tools"
fi
tools="$tools gvimdiff diffuse diffmerge ecmerge"
- tools="$tools p4merge araxis bc3 codecompare"
+ tools="$tools p4merge araxis bc bc3 codecompare"
fi
case "${VISUAL:-$EDITOR}" in
*vim*)
diff --git a/mergetools/bc3 b/mergetools/bc
similarity index 100%
rename from mergetools/bc3
rename to mergetools/bc
diff --git a/mergetools/bc3 b/mergetools/bc3
dissimilarity index 100%
index b6319d2..5d8dd48 100644
--- a/mergetools/bc3
+++ b/mergetools/bc3
@@ -1,25 +1 @@
-diff_cmd () {
- "$merge_tool_path" "$LOCAL" "$REMOTE"
-}
-
-merge_cmd () {
- touch "$BACKUP"
- if $base_present
- then
- "$merge_tool_path" "$LOCAL" "$REMOTE" "$BASE" \
- -mergeoutput="$MERGED"
- else
- "$merge_tool_path" "$LOCAL" "$REMOTE" \
- -mergeoutput="$MERGED"
- fi
- check_unchanged
-}
-
-translate_merge_tool_path() {
- if type bcomp >/dev/null 2>/dev/null
- then
- echo bcomp
- else
- echo bcompare
- fi
-}
+. "$MERGE_TOOLS_DIR/bc"

[Sebastian Schuberth]

Post by Junio C Hamano

Post by Junio C Hamano

Post by Olivier Croquette
Beyond compare 4 is out since september 2014. The CLI interface
doesn't seem to have changed compared to the version 3.

Hmph, if this is identical to mergetools/bc3, why is the patch even
needed? Do we auto-detect something and try to use bc4 which does
not exist and fail, and we must supply a copy as bc4 to prevent it?

The patch is indeed not needed, which is why I haven't cared to provide it so far although I'm now using Beyond Compare 4 instead of version 3 myself.

Post by Junio C Hamano

Post by Junio C Hamano
It may feel somewhat strange to have to say "mergetool --tool=bc3"
when you know what you have is version 4 and not version 3, but in

That's exactly the only reason I could think of why it could be nice to have a "bc4".

Post by Junio C Hamano

Post by Junio C Hamano
that case, I wonder if there are reasons why calling both versions
just "bc" is a bad idea. And assuming that version 5 and later

IMHO, the only reason not to just have a single "bc" is to maintain backward compatibility for users already using "bc3". But for the sake of cleaner code, personally I'd be fine with that minor backward compatibility breakage.

Post by Junio C Hamano

Post by Junio C Hamano
Perhaps version 2 and before are unusable as a mergetool backend or
something?

It seems that ffe6dc08 (mergetool--lib: Add Beyond Compare 3 as a
tool, 2011-02-27) is the first mention of "Beyond Compare" and it
only was interested in version 3 and nothing else.

Beyond Compare versions prior to 3 do not run on Linux, but only on Windows, which is why I did not care to submit a patch.

Post by Junio C Hamano
Perhaps something like this, so that existing users can still use
"bc3" and other people can use "bc" if it bothers them that they
have to say "3" when the backend driver works with both 3 and 4?

That indeed sounds like the best approach.

Post by Junio C Hamano
--- a/git-mergetool--lib.sh
+++ b/git-mergetool--lib.sh
@@ -250,7 +250,7 @@ list_merge_tool_candidates () {
tools="opendiff kdiff3 tkdiff xxdiff meld $tools"
fi
tools="$tools gvimdiff diffuse diffmerge ecmerge"
- tools="$tools p4merge araxis bc3 codecompare"
+ tools="$tools p4merge araxis bc bc3 codecompare"

Why keep bc3 here?

And shouldn't we update git-gui/lib/mergetool.tcl, too?

[Junio C Hamano]

Post by Sebastian Schuberth

Post by Junio C Hamano
Perhaps something like this, so that existing users can still use
"bc3" and other people can use "bc" if it bothers them that they
have to say "3" when the backend driver works with both 3 and 4?

That indeed sounds like the best approach.

Post by Junio C Hamano
--- a/git-mergetool--lib.sh
+++ b/git-mergetool--lib.sh
@@ -250,7 +250,7 @@ list_merge_tool_candidates () {
tools="opendiff kdiff3 tkdiff xxdiff meld $tools"
fi
tools="$tools gvimdiff diffuse diffmerge ecmerge"
- tools="$tools p4merge araxis bc3 codecompare"
+ tools="$tools p4merge araxis bc bc3 codecompare"

Why keep bc3 here?

I didn't carefully look at the code that uses this list to see if we
have to list everything or can list just the ones we recommend, and
erred on the safer side (unlike the one for completion where I
omitted bc3 as "deprecated").

I'll let mergetools experts decide when rolling the final patch ;-)

Post by Sebastian Schuberth
And shouldn't we update git-gui/lib/mergetool.tcl, too?

Yes we should, but git-gui is not in my bailiwick, and shouldn't be
done relative to my tree anyway. I'll Cc this message to Pat.

Thanks.

[David Aguilar]

Post by Junio C Hamano

Post by Sebastian Schuberth

Post by Junio C Hamano
Perhaps something like this, so that existing users can still use
"bc3" and other people can use "bc" if it bothers them that they
have to say "3" when the backend driver works with both 3 and 4?

That indeed sounds like the best approach.

Post by Junio C Hamano
--- a/git-mergetool--lib.sh
+++ b/git-mergetool--lib.sh
@@ -250,7 +250,7 @@ list_merge_tool_candidates () {
tools="opendiff kdiff3 tkdiff xxdiff meld $tools"
fi
tools="$tools gvimdiff diffuse diffmerge ecmerge"
- tools="$tools p4merge araxis bc3 codecompare"
+ tools="$tools p4merge araxis bc bc3 codecompare"

Why keep bc3 here?

I didn't carefully look at the code that uses this list to see if we
have to list everything or can list just the ones we recommend, and
erred on the safer side (unlike the one for completion where I
omitted bc3 as "deprecated").

We should drop "bc3" here. This is the list of candidates that
is used when no configuration exists. Listing "bc" twice here
implies that we might try that candidate twice.

Otherwise, this patch looks like the right way to go ~ it makes
"bc" available and keeps compatibility for "bc3".

If we wanted to phase out "bc3" for Git 3.0 we could start
warning inside of the "bc3" scriptlet, but I don't see any
reason to do so a.t.m.

Thanks,

[Junio C Hamano]

Post by David Aguilar
Otherwise, this patch looks like the right way to go ~ it makes
"bc" available and keeps compatibility for "bc3".

OK, thanks. Here is what I'll queue.

-- >8 --
From: Junio C Hamano <***@pobox.com>
Date: Mon, 20 Oct 2014 15:49:36 -0700
Subject: [PATCH] mergetool: rename bc3 to bc

Beyond Compare version 4 works the same way as version 3, so rename
the existing "bc3" adaptor to just "bc", while keeping "bc3" as a
backward compatible wrapper.

Noticed-by: Olivier Croquette <***@free.fr>
Helped-by: David Aguilar <***@gmail.com>
Signed-off-by: Junio C Hamano <***@pobox.com>
---
contrib/completion/git-completion.bash | 2 +-
git-mergetool--lib.sh | 2 +-
mergetools/bc | 25 +++++++++++++++++++++++++
mergetools/bc3 | 26 +-------------------------
4 files changed, 28 insertions(+), 27 deletions(-)
create mode 100644 mergetools/bc

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 06bf262..8a19b3e 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -1207,7 +1207,7 @@ _git_diff ()
}

__git_mergetools_common="diffuse diffmerge ecmerge emerge kdiff3 meld opendiff
- tkdiff vimdiff gvimdiff xxdiff araxis p4merge bc3 codecompare
+ tkdiff vimdiff gvimdiff xxdiff araxis p4merge bc codecompare
"

_git_difftool ()
diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh
index c45a020..a40d3df 100644
--- a/git-mergetool--lib.sh
+++ b/git-mergetool--lib.sh
@@ -250,7 +250,7 @@ list_merge_tool_candidates () {
tools="opendiff kdiff3 tkdiff xxdiff meld $tools"
fi
tools="$tools gvimdiff diffuse diffmerge ecmerge"
- tools="$tools p4merge araxis bc3 codecompare"
+ tools="$tools p4merge araxis bc codecompare"
fi
case "${VISUAL:-$EDITOR}" in
*vim*)
diff --git a/mergetools/bc b/mergetools/bc
new file mode 100644
index 0000000..b6319d2
--- /dev/null
+++ b/mergetools/bc
@@ -0,0 +1,25 @@
+diff_cmd () {
+ "$merge_tool_path" "$LOCAL" "$REMOTE"
+}
+
+merge_cmd () {
+ touch "$BACKUP"
+ if $base_present
+ then
+ "$merge_tool_path" "$LOCAL" "$REMOTE" "$BASE" \
+ -mergeoutput="$MERGED"
+ else
+ "$merge_tool_path" "$LOCAL" "$REMOTE" \
+ -mergeoutput="$MERGED"
+ fi
+ check_unchanged
+}
+
+translate_merge_tool_path() {
+ if type bcomp >/dev/null 2>/dev/null
+ then
+ echo bcomp
+ else
+ echo bcompare
+ fi
+}
diff --git a/mergetools/bc3 b/mergetools/bc3
index b6319d2..5d8dd48 100644
--- a/mergetools/bc3
+++ b/mergetools/bc3
@@ -1,25 +1 @@
-diff_cmd () {
- "$merge_tool_path" "$LOCAL" "$REMOTE"
-}
-
-merge_cmd () {
- touch "$BACKUP"
- if $base_present
- then
- "$merge_tool_path" "$LOCAL" "$REMOTE" "$BASE" \
- -mergeoutput="$MERGED"
- else
- "$merge_tool_path" "$LOCAL" "$REMOTE" \
- -mergeoutput="$MERGED"
- fi
- check_unchanged
-}
-
-translate_merge_tool_path() {
- if type bcomp >/dev/null 2>/dev/null
- then
- echo bcomp
- else
- echo bcompare
- fi
-}
+. "$MERGE_TOOLS_DIR/bc"

[Olivier Croquette]

Post by Junio C Hamano
Perhaps something like this, so that existing users can still use
"bc3" and other people can use "bc" if it bothers them that they
have to say "3" when the backend driver works with both 3 and 4?

Thanks for the quick and great feedback.
This looks good to me. I used =93bc4" just for consistency sake and to =
preserve backwards compatibility, but introducing =93bc=94 as a generic=
backend looks indeed as a better approach.
It=92s even future safe, because it doesn=92t prevent introducing bc5 i=
f required later on.

Olivier