threads / patch / 18806

v2, 2 partsEnsure consistent usage of mergetool.keepBackup in git-gui

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

## tl;dr

9 messages between Apr 9, 2009 and Apr 11, 2009. Diffs are folded; open one to read it.

replies: 8people: 4as markdown or json

Ferry Huberts· Apr 9, 2009, 15:45 UTC · lore

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

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(-)
Show changes to 2 files +2 −2

contrib/difftool/git-difftool.txt, git-mergetool.sh

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
Ferry Huberts· Apr 9, 2009, 15:45 UTC · re: Ferry Huberts · lore

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(-)
Show changes to 2 files +2 −2

git-gui/git-gui.sh, git-gui/lib/mergetool.tcl

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
David Aguilar· Apr 10, 2009, 03:27 UTC · re: Ferry Huberts · lore

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

Hi
On  0, Ferry Huberts <ferry.huberts@pelagic.nl> wrote:
Show 9 quoted lines
> 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
Ferry Huberts (Pelagic)· Apr 10, 2009, 06:58 UTC · re: David Aguilar · lore

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

David Aguilar wrote:
Show 17 quoted lines
> 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
Show 17 quoted lines
> 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
Show 20 quoted lines
> 
> 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?
David Aguilar· Apr 10, 2009, 07:43 UTC · re: Ferry Huberts (Pelagic) · lore

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

On  0, "Ferry Huberts (Pelagic)" <ferry.huberts@pelagic.nl> wrote:
Show 28 quoted lines
> 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
David Aguilar· Apr 10, 2009, 08:18 UTC · re: David Aguilar · lore

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

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
Ferry Huberts (Pelagic)· Apr 10, 2009, 08:25 UTC · re: David Aguilar · lore

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

David Aguilar wrote:
Show 37 quoted lines
> 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
Markus Heidelberg· Apr 10, 2009, 14:48 UTC · re: David Aguilar · lore

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

David Aguilar, 10.04.2009:
Show 6 quoted lines
> 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.

Show 20 quoted lines
> $ 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
Charles Bailey· Apr 11, 2009, 12:04 UTC · re: David Aguilar · lore

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

On Fri, Apr 10, 2009 at 01:18:44AM -0700, David Aguilar wrote:
Show 27 quoted lines
> 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/

← back to recent threads