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

From: Ferry Huberts (Pelagic) <ferry.huberts@pelagic.nl>
Date: Apr 10, 2009, 06:58 UTC
Message-ID: <49DEEE22.5030500@pelagic.nl>
In-Reply-To: <20090410032731.GA1545@gmail.com>

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?