[[PATCH 1/1] Ensure consistent usage of mergetool.keepBackup

[[PATCH 1/1] Ensure consistent usage of mergetool.keepBackup

[Ferry Huberts]

In several places merge.keepBackup is used i.s.o.
mergetool.keepBackup. This patch makes it all
consistent.

Signed-off-by: Ferry Huberts <ferry.huberts@pelagic.nl>
---
This patch is rebased on v1.6.2.2

 contrib/difftool/git-difftool.txt |    2 +-
 git-gui/git-gui.sh                |    2 +-
 git-gui/lib/mergetool.tcl         |    2 +-
 git-mergetool.sh                  |    2 +-
 4 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/contrib/difftool/git-difftool.txt b/contrib/difftool/git-difftool.txt
index 6e2610c..ca7482a 100644
--- a/contrib/difftool/git-difftool.txt
+++ b/contrib/difftool/git-difftool.txt
@@ -66,7 +66,7 @@ merge.tool::
 +
 See the `--tool=<tool>` option above for more details.
 
-merge.keepBackup::
+mergetool.keepBackup::
 	The original, unedited file content can be saved to a file with
 	a `.orig` extension.  Defaults to `true` (i.e. keep the backup files).
 
diff --git a/git-gui/git-gui.sh b/git-gui/git-gui.sh
index e018e07..e4e643a 100755
--- a/git-gui/git-gui.sh
+++ b/git-gui/git-gui.sh
@@ -699,7 +699,7 @@ proc apply_config {} {
 
 set default_config(branch.autosetupmerge) true
 set default_config(merge.tool) {}
-set default_config(merge.keepbackup) true
+set default_config(mergetool.keepbackup) true
 set default_config(merge.diffstat) true
 set default_config(merge.summary) false
 set default_config(merge.verbosity) 2
diff --git a/git-gui/lib/mergetool.tcl b/git-gui/lib/mergetool.tcl
index eb2b4b5..b7263b3 100644
--- a/git-gui/lib/mergetool.tcl
+++ b/git-gui/lib/mergetool.tcl
@@ -382,7 +382,7 @@ proc merge_tool_finish {fd} {
 		delete_temp_files $mtool_tmpfiles
 		ui_status [mc "Merge tool failed."]
 	} else {
-		if {[is_config_true merge.keepbackup]} {
+		if {[is_config_true mergetool.keepbackup]} {
 			file rename -force -- $backup "$mtool_target.orig"
 		}
 
diff --git a/git-mergetool.sh b/git-mergetool.sh
index 87fa88a..1455bd9 100755
--- a/git-mergetool.sh
+++ b/git-mergetool.sh
@@ -430,7 +430,7 @@ else
 
     init_merge_tool_path "$merge_tool"
 
-    merge_keep_backup="$(git config --bool merge.keepBackup || echo true)"
+    merge_keep_backup="$(git config --bool mergetool.keepBackup || echo true)"
     merge_keep_temporaries="$(git config --bool mergetool.keepTemporaries || echo false)"
 
     if test -z "$merge_tool_cmd" && ! type "$merge_tool_path" > /dev/null 2>&1; then
-- 
1.6.0.6

Re: RESEND [PATCH 1/1] Ensure consistent usage of mergetool.keepBackup

[Shawn O. Pearce]

Ferry Huberts <ferry.huberts@pelagic.nl> wrote:
> In several places merge.keepBackup is used i.s.o.
> mergetool.keepBackup. This patch makes it all
> consistent.
> 
> Signed-off-by: Ferry Huberts <ferry.huberts@pelagic.nl>
> ---
> This patch is rebased on v1.6.2.2
> 
>  contrib/difftool/git-difftool.txt |    2 +-
>  git-gui/git-gui.sh                |    2 +-
>  git-gui/lib/mergetool.tcl         |    2 +-
>  git-mergetool.sh                  |    2 +-
>  4 files changed, 4 insertions(+), 4 deletions(-)

git-gui patches should be sent by themselves as they apply to
git-gui.git which is then merged into git.git every so often.

Yesterday I broke the git-gui part out by hand and applied it to
git-gui.git.  You should break this patch down and send Junio the
non-git-gui half so he can more easily apply it to git.git.
 
-- 
Shawn.

[PATCH v2 1/2] Ensure consistent usage of mergetool.keepBackup in git

[Ferry Huberts]

In several places merge.keepBackup is used i.s.o.
mergetool.keepBackup. This patch makes it all
consistent for git

Signed-off-by: Ferry Huberts <ferry.huberts@pelagic.nl>
---
 contrib/difftool/git-difftool.txt |    2 +-
 git-mergetool.sh                  |    2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/contrib/difftool/git-difftool.txt b/contrib/difftool/git-difftool.txt
index 6e2610c..ca7482a 100644
--- a/contrib/difftool/git-difftool.txt
+++ b/contrib/difftool/git-difftool.txt
@@ -66,7 +66,7 @@ merge.tool::
 +
 See the `--tool=<tool>` option above for more details.
 
-merge.keepBackup::
+mergetool.keepBackup::
 	The original, unedited file content can be saved to a file with
 	a `.orig` extension.  Defaults to `true` (i.e. keep the backup files).
 
diff --git a/git-mergetool.sh b/git-mergetool.sh
index 87fa88a..1455bd9 100755
--- a/git-mergetool.sh
+++ b/git-mergetool.sh
@@ -430,7 +430,7 @@ else
 
     init_merge_tool_path "$merge_tool"
 
-    merge_keep_backup="$(git config --bool merge.keepBackup || echo true)"
+    merge_keep_backup="$(git config --bool mergetool.keepBackup || echo true)"
     merge_keep_temporaries="$(git config --bool mergetool.keepTemporaries || echo false)"
 
     if test -z "$merge_tool_cmd" && ! type "$merge_tool_path" > /dev/null 2>&1; then
-- 
1.6.0.6

[PATCH v2 2/2] Ensure consistent usage of mergetool.keepBackup in git-gui

[Ferry Huberts]

In several places merge.keepBackup is used i.s.o.
mergetool.keepBackup. This patch makes it all
consistent for git-gui

Signed-off-by: Ferry Huberts <ferry.huberts@pelagic.nl>
---
 git-gui/git-gui.sh        |    2 +-
 git-gui/lib/mergetool.tcl |    2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/git-gui/git-gui.sh b/git-gui/git-gui.sh
index e018e07..e4e643a 100755
--- a/git-gui/git-gui.sh
+++ b/git-gui/git-gui.sh
@@ -699,7 +699,7 @@ proc apply_config {} {
 
 set default_config(branch.autosetupmerge) true
 set default_config(merge.tool) {}
-set default_config(merge.keepbackup) true
+set default_config(mergetool.keepbackup) true
 set default_config(merge.diffstat) true
 set default_config(merge.summary) false
 set default_config(merge.verbosity) 2
diff --git a/git-gui/lib/mergetool.tcl b/git-gui/lib/mergetool.tcl
index eb2b4b5..b7263b3 100644
--- a/git-gui/lib/mergetool.tcl
+++ b/git-gui/lib/mergetool.tcl
@@ -382,7 +382,7 @@ proc merge_tool_finish {fd} {
 		delete_temp_files $mtool_tmpfiles
 		ui_status [mc "Merge tool failed."]
 	} else {
-		if {[is_config_true merge.keepbackup]} {
+		if {[is_config_true mergetool.keepbackup]} {
 			file rename -force -- $backup "$mtool_target.orig"
 		}
 
-- 
1.6.0.6

Re: [PATCH v2 1/2] Ensure consistent usage of mergetool.keepBackup in git

[David Aguilar]

Hi

On  0, Ferry Huberts <ferry.huberts@pelagic.nl> wrote:
> In several places merge.keepBackup is used i.s.o.
> mergetool.keepBackup. This patch makes it all
> consistent for git
> 
> Signed-off-by: Ferry Huberts <ferry.huberts@pelagic.nl>
> ---
>  contrib/difftool/git-difftool.txt |    2 +-
>  git-mergetool.sh                  |    2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)


You missed the usage of merge.keepBackup in
contrib/difftool/git-difftool-helper.

But.. (for better or worse) the "keep backup" feature
was completely removed from difftool, so patching this for
difftool now isn't very useful.

See 2e8af7e4 which is currently in origin/pu:

	difftool: remove the backup file feature


Also, we just got through a very large round of
mergetool/difftool changes.  The latest version is sitting in
Junio's pu branch.  The "da/difftool" branch's head is
currently pointing at 21d0ba7e.

It might be worth basing your work on top of that series since
that'd make things much easier to merge.


My only other comment is:

Aside from git-gui, there are other scripts out there that
use merge.keepBackup instead of mergetool.keepBackup,
so changing the name of the config variable has negligable
user benefit and will cause problems for people that:

A) already have merge.keepBackup configured and then get
surprised when one day all of a sudden git starts leaving
around these ".orig turds" (technical term, overheard from an
actual user) for no apparent reason even though they had
already configured it to do otherwise in the past, or

B) have written GUIs or scripts that know about
merge.keepBackup.

Aside from the naming, there's little user benefit to this
change in my opinion.

Anyways, just my thoughts.

-- 

	David

Re: [PATCH v2 1/2] Ensure consistent usage of mergetool.keepBackup in git

[Ferry Huberts (Pelagic)]

David Aguilar wrote:
> Hi
> 
> On  0, Ferry Huberts <ferry.huberts@pelagic.nl> wrote:
>> In several places merge.keepBackup is used i.s.o.
>> mergetool.keepBackup. This patch makes it all
>> consistent for git
>>
>> Signed-off-by: Ferry Huberts <ferry.huberts@pelagic.nl>
>> ---
>>  contrib/difftool/git-difftool.txt |    2 +-
>>  git-mergetool.sh                  |    2 +-
>>  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> 
> You missed the usage of merge.keepBackup in
> contrib/difftool/git-difftool-helper.
> 

I did miss it. didn't read the grep good enough I guess. thanks

> But.. (for better or worse) the "keep backup" feature
> was completely removed from difftool, so patching this for
> difftool now isn't very useful.
> 
> See 2e8af7e4 which is currently in origin/pu:
> 
> 	difftool: remove the backup file feature
> 
> 
> Also, we just got through a very large round of
> mergetool/difftool changes.  The latest version is sitting in
> Junio's pu branch.  The "da/difftool" branch's head is
> currently pointing at 21d0ba7e.
> 
> It might be worth basing your work on top of that series since
> that'd make things much easier to merge.
> 

on pu? I'll do that

> 
> My only other comment is:
> 
> Aside from git-gui, there are other scripts out there that
> use merge.keepBackup instead of mergetool.keepBackup,
> so changing the name of the config variable has negligable
> user benefit and will cause problems for people that:
> 
> A) already have merge.keepBackup configured and then get
> surprised when one day all of a sudden git starts leaving
> around these ".orig turds" (technical term, overheard from an
> actual user) for no apparent reason even though they had
> already configured it to do otherwise in the past, or
> 
> B) have written GUIs or scripts that know about
> merge.keepBackup.
> 
> Aside from the naming, there's little user benefit to this
> change in my opinion.
> 

I patched it this way because contrib/completion/git-completion.bash and
Documentation/config.txt talk about mergetool.keepBackup while only
contrib/difftool/git-difftool.txt talks about merge.keepBackup. That
seemed the most logical way of doing it.

I agree that some users might be surprised after this patch, otoh: I was
quite surprised that I still had turds even when I set
mergetool.keepBackup, which is what the documentation told me to do :-)
Do we really want to keep using 2 names for the same thing?

[rebasing now...]

I'm seeing the following grep on pu:

contrib/completion/git-completion.bash:	mergetool.keepBackup
Documentation/config.txt:mergetool.keepBackup::
git-gui/lib/mergetool.tcl:if {[is_config_true merge.keepbackup]} {
git-gui/git-gui:set default_config(merge.keepbackup) true
git-gui/git-gui.sh:set default_config(merge.keepbackup) true
git-mergetool.sh:merge_keep_backup="$(git config --bool merge.keepBackup
|| echo true)"

So it seems that merge.keepBackup is actually used consistently in the
code while the completion and documentation talk about mergetool.keepBackup.

Shall I just patch the completion and documentation instead?

Re: [PATCH v2 1/2] Ensure consistent usage of mergetool.keepBackup in git

[David Aguilar]

On  0, "Ferry Huberts (Pelagic)" <ferry.huberts@pelagic.nl> wrote:
> David Aguilar wrote:
> 
> I patched it this way because contrib/completion/git-completion.bash and
> Documentation/config.txt talk about mergetool.keepBackup while only
> contrib/difftool/git-difftool.txt talks about merge.keepBackup. That
> seemed the most logical way of doing it.
> 
> I agree that some users might be surprised after this patch, otoh: I was
> quite surprised that I still had turds even when I set
> mergetool.keepBackup, which is what the documentation told me to do :-)
> Do we really want to keep using 2 names for the same thing?
> 
> [rebasing now...]
> 
> I'm seeing the following grep on pu:
> 
> contrib/completion/git-completion.bash:	mergetool.keepBackup
> Documentation/config.txt:mergetool.keepBackup::
> git-gui/lib/mergetool.tcl:if {[is_config_true merge.keepbackup]} {
> git-gui/git-gui:set default_config(merge.keepbackup) true
> git-gui/git-gui.sh:set default_config(merge.keepbackup) true
> git-mergetool.sh:merge_keep_backup="$(git config --bool merge.keepBackup
> || echo true)"
> 
> So it seems that merge.keepBackup is actually used consistently in the
> code while the completion and documentation talk about mergetool.keepBackup.
> 
> Shall I just patch the completion and documentation instead?


Oh wow thanks for your investigation.
It /seems/ like the docs and completion should be updated.
That's funny.. I guess it's so subtle it just passed by
unnoticed this whole time.

-- 

	David

Re: [PATCH v2 1/2] Ensure consistent usage of mergetool.keepBackup in git

[David Aguilar]

On  0, David Aguilar <davvid@gmail.com> wrote:
> 
> It /seems/ like the docs and completion should be updated.

Though my guess is as good as any....
I'd rather hear someone else's opinion.

$ git log -p 44c36d1c
commit 44c36d1ccc9a40bfb31910dfd7e18d59fa8be502
Author: Charles Bailey <charles@hashpling.org>
Date:   Thu Feb 21 23:30:02 2008 +0000

    Tidy up git mergetool's backup file behaviour
    
    Currently a backup pre-merge file with conflict markers is sometimes
    kept with a .orig extenstion and sometimes removed depending on the
    particular merge tool used.
    
    This patch makes the handling consistent across all merge tools and
    configurable via a new mergetool.keepBackup config variable
    
    Signed-off-by: Charles Bailey <charles@hashpling.org>
    Signed-off-by: Junio C Hamano <gitster@pobox.com>


The commit comment says mergetool.keepBackup, even though the code always
had it as merge.keepBackup.

$ git log -p 7e30682c

Right now more people have merge.keepbackup already set since git-gui
has had it that way for the last 7 months or so.  Nevertheless,
Shawn's already applied the git-gui patch which hints that maybe
we should just make the code match the docs.  In which case, a
patch against pu would be a good thing, but I would like to
hear someone else's opinion just so that you don't waste time
going down the wrong route.

-- 

	David

Re: [PATCH v2 1/2] Ensure consistent usage of mergetool.keepBackup in git

[Ferry Huberts (Pelagic)]

David Aguilar wrote:
> On  0, David Aguilar <davvid@gmail.com> wrote:
>> It /seems/ like the docs and completion should be updated.
> 
> Though my guess is as good as any....
> I'd rather hear someone else's opinion.
> 
> $ git log -p 44c36d1c
> commit 44c36d1ccc9a40bfb31910dfd7e18d59fa8be502
> Author: Charles Bailey <charles@hashpling.org>
> Date:   Thu Feb 21 23:30:02 2008 +0000
> 
>     Tidy up git mergetool's backup file behaviour
>     
>     Currently a backup pre-merge file with conflict markers is sometimes
>     kept with a .orig extenstion and sometimes removed depending on the
>     particular merge tool used.
>     
>     This patch makes the handling consistent across all merge tools and
>     configurable via a new mergetool.keepBackup config variable
>     
>     Signed-off-by: Charles Bailey <charles@hashpling.org>
>     Signed-off-by: Junio C Hamano <gitster@pobox.com>
> 
> 
> The commit comment says mergetool.keepBackup, even though the code always
> had it as merge.keepBackup.
> 
> $ git log -p 7e30682c
> 
> Right now more people have merge.keepbackup already set since git-gui
> has had it that way for the last 7 months or so.  Nevertheless,
> Shawn's already applied the git-gui patch which hints that maybe
> we should just make the code match the docs.  In which case, a
> patch against pu would be a good thing, but I would like to
> hear someone else's opinion just so that you don't waste time
> going down the wrong route.
> 
agree

Re: [PATCH v2 1/2] Ensure consistent usage of mergetool.keepBackup in git

[Markus Heidelberg]

David Aguilar, 10.04.2009:
> On  0, David Aguilar <davvid@gmail.com> wrote:
> > 
> > It /seems/ like the docs and completion should be updated.
> 
> Though my guess is as good as any....
> I'd rather hear someone else's opinion.

And I've waited for a response from the difftool/mergetool maintainers
at first, so I waited with my reply till now.

> $ git log -p 44c36d1c
> commit 44c36d1ccc9a40bfb31910dfd7e18d59fa8be502
> Author: Charles Bailey <charles@hashpling.org>
> Date:   Thu Feb 21 23:30:02 2008 +0000
> 
>     Tidy up git mergetool's backup file behaviour
>     
>     Currently a backup pre-merge file with conflict markers is sometimes
>     kept with a .orig extenstion and sometimes removed depending on the
>     particular merge tool used.
>     
>     This patch makes the handling consistent across all merge tools and
>     configurable via a new mergetool.keepBackup config variable
>     
>     Signed-off-by: Charles Bailey <charles@hashpling.org>
>     Signed-off-by: Junio C Hamano <gitster@pobox.com>
> 
> 
> The commit comment says mergetool.keepBackup, even though the code always
> had it as merge.keepBackup.

Yes, merge.keepbackup was never documented, we shouldn't have to keep
backwards compatibility for such undocumented behaviour.

> Right now more people have merge.keepbackup already set since git-gui
> has had it that way for the last 7 months or so.

People which noticed that the documented mergetool.keepBackup didn't
work, had to look into the source to find out that merge.keepBackup did
it, so I see no problem with changing it. They rather should have
reported it anyway.

Ferry did it right with fixing this inconsistency.

> Nevertheless,
> Shawn's already applied the git-gui patch which hints that maybe
> we should just make the code match the docs.

Yes.

Markus

Re: [PATCH v2 1/2] Ensure consistent usage of mergetool.keepBackup in git

[Charles Bailey]

On Fri, Apr 10, 2009 at 01:18:44AM -0700, David Aguilar wrote:
> On  0, David Aguilar <davvid@gmail.com> wrote:
> > 
> > It /seems/ like the docs and completion should be updated.
> 
> Though my guess is as good as any....
> I'd rather hear someone else's opinion.
> 
> $ git log -p 44c36d1c
> commit 44c36d1ccc9a40bfb31910dfd7e18d59fa8be502
> Author: Charles Bailey <charles@hashpling.org>
> Date:   Thu Feb 21 23:30:02 2008 +0000
> 
>     Tidy up git mergetool's backup file behaviour
>     
>     Currently a backup pre-merge file with conflict markers is sometimes
>     kept with a .orig extenstion and sometimes removed depending on the
>     particular merge tool used.
>     
>     This patch makes the handling consistent across all merge tools and
>     configurable via a new mergetool.keepBackup config variable
>     
>     Signed-off-by: Charles Bailey <charles@hashpling.org>
>     Signed-off-by: Junio C Hamano <gitster@pobox.com>
> 
> 
> The commit comment says mergetool.keepBackup, even though the code always
> had it as merge.keepBackup.

Not going by the commit message, but rather by the documentation I do
believe that mergetool.keepBackup should have been the correct
variable to use. I'm not sure why I missed this at the time.

I seem to remember looking for existing consistency in this before.
Originally I was going on the premise that merge.* were things that
were general between merges and all mergetools whereas mergetool.*
were things that were merge tool specific. Subsequently, it was more
natural to have mergetool.$tool.* for things that were tool specifit
and to have mergetool.* as general mergetool specific settings,
leaving merge.* as merge specific settings with the exception of
merge.tool which is obviously specific to git mergetool, but is in the
merge.* section.

-- 
Charles Bailey
http://ccgi.hashpling.plus.com/blog/