{"thread":{"id":"18806","subject":"[PATCH v2 2/2] Ensure consistent usage of mergetool.keepBackup in git-gui","startedAt":"2009-04-09T15:45:10Z","lastAt":"2009-04-11T12:04:35Z","messageCount":9,"participants":["Ferry Huberts","David Aguilar","Ferry Huberts (Pelagic)","Markus Heidelberg","Charles Bailey"],"isPatch":true,"patchVersion":2,"patchTotal":2},"messages":[{"id":"110925","messageId":"f6297e57a23dc3abac2fcedceb00cecde607de02.1239291673.git.ferry.huberts@pelagic.nl","threadId":"18806","inReplyTo":"20090409153033.GN23604@spearce.org","subject":"[PATCH v2 1/2] Ensure consistent usage of mergetool.keepBackup in git","fromName":"Ferry Huberts","fromEmail":"ferry.huberts@pelagic.nl","sentAt":"2009-04-09T15:45:10Z","receivedAt":"2009-04-09T15:45:10Z","isPatch":true,"sender":{"key":"ferry.huberts@pelagic.nl","avatar":"https://gravatar.com/avatar/9f63c0289ad23cbdef0f7609a0af85ff0f4b3babfd066de9ff58f62d48cfd6f2?d=mp&s=160"},"body":"In several places merge.keepBackup is used i.s.o.\nmergetool.keepBackup. This patch makes it all\nconsistent for git\n\nSigned-off-by: Ferry Huberts \n---\n contrib/difftool/git-difftool.txt | 2 +-\n git-mergetool.sh | 2 +-\n 2 files changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/contrib/difftool/git-difftool.txt b/contrib/difftool/git-difftool.txt\nindex 6e2610c..ca7482a 100644\n--- a/contrib/difftool/git-difftool.txt\n+++ b/contrib/difftool/git-difftool.txt\n@@ -66,7 +66,7 @@ merge.tool::\n +\n See the `--tool=` option above for more details.\n \n-merge.keepBackup::\n+mergetool.keepBackup::\n \tThe original, unedited file content can be saved to a file with\n \ta `.orig` extension. Defaults to `true` (i.e. keep the backup files).\n \ndiff --git a/git-mergetool.sh b/git-mergetool.sh\nindex 87fa88a..1455bd9 100755\n--- a/git-mergetool.sh\n+++ b/git-mergetool.sh\n@@ -430,7 +430,7 @@ else\n \n init_merge_tool_path \"$merge_tool\"\n \n- merge_keep_backup=\"$(git config --bool merge.keepBackup || echo true)\"\n+ merge_keep_backup=\"$(git config --bool mergetool.keepBackup || echo true)\"\n merge_keep_temporaries=\"$(git config --bool mergetool.keepTemporaries || echo false)\"\n \n if test -z \"$merge_tool_cmd\" && ! type \"$merge_tool_path\" > /dev/null 2>&1; then\n-- \n1.6.0.6\n"},{"id":"110924","messageId":"7a755d80914df2ebc1a7dc349ed215bae9ffec34.1239291673.git.ferry.huberts@pelagic.nl","threadId":"18806","inReplyTo":"f6297e57a23dc3abac2fcedceb00cecde607de02.1239291673.git.ferry.huberts@pelagic.nl","subject":"[PATCH v2 2/2] Ensure consistent usage of mergetool.keepBackup in git-gui","fromName":"Ferry Huberts","fromEmail":"ferry.huberts@pelagic.nl","sentAt":"2009-04-09T15:45:11Z","receivedAt":"2009-04-09T15:45:11Z","isPatch":true,"sender":{"key":"ferry.huberts@pelagic.nl","avatar":"https://gravatar.com/avatar/9f63c0289ad23cbdef0f7609a0af85ff0f4b3babfd066de9ff58f62d48cfd6f2?d=mp&s=160"},"body":"In several places merge.keepBackup is used i.s.o.\nmergetool.keepBackup. This patch makes it all\nconsistent for git-gui\n\nSigned-off-by: Ferry Huberts \n---\n git-gui/git-gui.sh | 2 +-\n git-gui/lib/mergetool.tcl | 2 +-\n 2 files changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/git-gui/git-gui.sh b/git-gui/git-gui.sh\nindex e018e07..e4e643a 100755\n--- a/git-gui/git-gui.sh\n+++ b/git-gui/git-gui.sh\n@@ -699,7 +699,7 @@ proc apply_config {} {\n \n set default_config(branch.autosetupmerge) true\n set default_config(merge.tool) {}\n-set default_config(merge.keepbackup) true\n+set default_config(mergetool.keepbackup) true\n set default_config(merge.diffstat) true\n set default_config(merge.summary) false\n set default_config(merge.verbosity) 2\ndiff --git a/git-gui/lib/mergetool.tcl b/git-gui/lib/mergetool.tcl\nindex eb2b4b5..b7263b3 100644\n--- a/git-gui/lib/mergetool.tcl\n+++ b/git-gui/lib/mergetool.tcl\n@@ -382,7 +382,7 @@ proc merge_tool_finish {fd} {\n \t\tdelete_temp_files $mtool_tmpfiles\n \t\tui_status [mc \"Merge tool failed.\"]\n \t} else {\n-\t\tif {[is_config_true merge.keepbackup]} {\n+\t\tif {[is_config_true mergetool.keepbackup]} {\n \t\t\tfile rename -force -- $backup \"$mtool_target.orig\"\n \t\t}\n \n-- \n1.6.0.6\n"},{"id":"110986","messageId":"20090410032731.GA1545@gmail.com","threadId":"18806","inReplyTo":"f6297e57a23dc3abac2fcedceb00cecde607de02.1239291673.git.ferry.huberts@pelagic.nl","subject":"Re: [PATCH v2 1/2] Ensure consistent usage of mergetool.keepBackup in git","fromName":"David Aguilar","fromEmail":"davvid@gmail.com","sentAt":"2009-04-10T03:27:32Z","receivedAt":"2009-04-10T03:27:32Z","isPatch":true,"sender":{"key":"davvid@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13196?v=4"},"body":"\nHi\n\nOn 0, Ferry Huberts wrote:\n> In several places merge.keepBackup is used i.s.o.\n> mergetool.keepBackup. This patch makes it all\n> consistent for git\n> \n> Signed-off-by: Ferry Huberts \n> ---\n> contrib/difftool/git-difftool.txt | 2 +-\n> git-mergetool.sh | 2 +-\n> 2 files changed, 2 insertions(+), 2 deletions(-)\n\n\nYou missed the usage of merge.keepBackup in\ncontrib/difftool/git-difftool-helper.\n\nBut.. (for better or worse) the \"keep backup\" feature\nwas completely removed from difftool, so patching this for\ndifftool now isn't very useful.\n\nSee 2e8af7e4 which is currently in origin/pu:\n\n\tdifftool: remove the backup file feature\n\n\nAlso, we just got through a very large round of\nmergetool/difftool changes. The latest version is sitting in\nJunio's pu branch. The \"da/difftool\" branch's head is\ncurrently pointing at 21d0ba7e.\n\nIt might be worth basing your work on top of that series since\nthat'd make things much easier to merge.\n\n\nMy only other comment is:\n\nAside from git-gui, there are other scripts out there that\nuse merge.keepBackup instead of mergetool.keepBackup,\nso changing the name of the config variable has negligable\nuser benefit and will cause problems for people that:\n\nA) already have merge.keepBackup configured and then get\nsurprised when one day all of a sudden git starts leaving\naround these \".orig turds\" (technical term, overheard from an\nactual user) for no apparent reason even though they had\nalready configured it to do otherwise in the past, or\n\nB) have written GUIs or scripts that know about\nmerge.keepBackup.\n\nAside from the naming, there's little user benefit to this\nchange in my opinion.\n\nAnyways, just my thoughts.\n\n-- \n\n\tDavid\n"},{"id":"110990","messageId":"49DEEE22.5030500@pelagic.nl","threadId":"18806","inReplyTo":"20090410032731.GA1545@gmail.com","subject":"Re: [PATCH v2 1/2] Ensure consistent usage of mergetool.keepBackup in git","fromName":"Ferry Huberts (Pelagic)","fromEmail":"ferry.huberts@pelagic.nl","sentAt":"2009-04-10T06:58:42Z","receivedAt":"2009-04-10T06:58:42Z","isPatch":true,"sender":{"key":"ferry.huberts@pelagic.nl","avatar":"https://gravatar.com/avatar/9f63c0289ad23cbdef0f7609a0af85ff0f4b3babfd066de9ff58f62d48cfd6f2?d=mp&s=160"},"body":"David Aguilar wrote:\n> Hi\n> \n> On 0, Ferry Huberts wrote:\n>> In several places merge.keepBackup is used i.s.o.\n>> mergetool.keepBackup. This patch makes it all\n>> consistent for git\n>>\n>> Signed-off-by: Ferry Huberts \n>> ---\n>> contrib/difftool/git-difftool.txt | 2 +-\n>> git-mergetool.sh | 2 +-\n>> 2 files changed, 2 insertions(+), 2 deletions(-)\n> \n> \n> You missed the usage of merge.keepBackup in\n> contrib/difftool/git-difftool-helper.\n> \n\nI did miss it. didn't read the grep good enough I guess. thanks\n\n> But.. (for better or worse) the \"keep backup\" feature\n> was completely removed from difftool, so patching this for\n> difftool now isn't very useful.\n> \n> See 2e8af7e4 which is currently in origin/pu:\n> \n> \tdifftool: remove the backup file feature\n> \n> \n> Also, we just got through a very large round of\n> mergetool/difftool changes. The latest version is sitting in\n> Junio's pu branch. The \"da/difftool\" branch's head is\n> currently pointing at 21d0ba7e.\n> \n> It might be worth basing your work on top of that series since\n> that'd make things much easier to merge.\n> \n\non pu? I'll do that\n\n> \n> My only other comment is:\n> \n> Aside from git-gui, there are other scripts out there that\n> use merge.keepBackup instead of mergetool.keepBackup,\n> so changing the name of the config variable has negligable\n> user benefit and will cause problems for people that:\n> \n> A) already have merge.keepBackup configured and then get\n> surprised when one day all of a sudden git starts leaving\n> around these \".orig turds\" (technical term, overheard from an\n> actual user) for no apparent reason even though they had\n> already configured it to do otherwise in the past, or\n> \n> B) have written GUIs or scripts that know about\n> merge.keepBackup.\n> \n> Aside from the naming, there's little user benefit to this\n> change in my opinion.\n> \n\nI patched it this way because contrib/completion/git-completion.bash and\nDocumentation/config.txt talk about mergetool.keepBackup while only\ncontrib/difftool/git-difftool.txt talks about merge.keepBackup. That\nseemed the most logical way of doing it.\n\nI agree that some users might be surprised after this patch, otoh: I was\nquite surprised that I still had turds even when I set\nmergetool.keepBackup, which is what the documentation told me to do :-)\nDo we really want to keep using 2 names for the same thing?\n\n[rebasing now...]\n\nI'm seeing the following grep on pu:\n\ncontrib/completion/git-completion.bash:\tmergetool.keepBackup\nDocumentation/config.txt:mergetool.keepBackup::\ngit-gui/lib/mergetool.tcl:if {[is_config_true merge.keepbackup]} {\ngit-gui/git-gui:set default_config(merge.keepbackup) true\ngit-gui/git-gui.sh:set default_config(merge.keepbackup) true\ngit-mergetool.sh:merge_keep_backup=\"$(git config --bool merge.keepBackup\n|| echo true)\"\n\nSo it seems that merge.keepBackup is actually used consistently in the\ncode while the completion and documentation talk about mergetool.keepBackup.\n\nShall I just patch the completion and documentation instead?\n"},{"id":"110992","messageId":"20090410074327.GA9369@gmail.com","threadId":"18806","inReplyTo":"49DEEE22.5030500@pelagic.nl","subject":"Re: [PATCH v2 1/2] Ensure consistent usage of mergetool.keepBackup in git","fromName":"David Aguilar","fromEmail":"davvid@gmail.com","sentAt":"2009-04-10T07:43:29Z","receivedAt":"2009-04-10T07:43:29Z","isPatch":true,"sender":{"key":"davvid@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13196?v=4"},"body":"On 0, \"Ferry Huberts (Pelagic)\" wrote:\n> David Aguilar wrote:\n> \n> I patched it this way because contrib/completion/git-completion.bash and\n> Documentation/config.txt talk about mergetool.keepBackup while only\n> contrib/difftool/git-difftool.txt talks about merge.keepBackup. That\n> seemed the most logical way of doing it.\n> \n> I agree that some users might be surprised after this patch, otoh: I was\n> quite surprised that I still had turds even when I set\n> mergetool.keepBackup, which is what the documentation told me to do :-)\n> Do we really want to keep using 2 names for the same thing?\n> \n> [rebasing now...]\n> \n> I'm seeing the following grep on pu:\n> \n> contrib/completion/git-completion.bash:\tmergetool.keepBackup\n> Documentation/config.txt:mergetool.keepBackup::\n> git-gui/lib/mergetool.tcl:if {[is_config_true merge.keepbackup]} {\n> git-gui/git-gui:set default_config(merge.keepbackup) true\n> git-gui/git-gui.sh:set default_config(merge.keepbackup) true\n> git-mergetool.sh:merge_keep_backup=\"$(git config --bool merge.keepBackup\n> || echo true)\"\n> \n> So it seems that merge.keepBackup is actually used consistently in the\n> code while the completion and documentation talk about mergetool.keepBackup.\n> \n> Shall I just patch the completion and documentation instead?\n\n\nOh wow thanks for your investigation.\nIt /seems/ like the docs and completion should be updated.\nThat's funny.. I guess it's so subtle it just passed by\nunnoticed this whole time.\n\n-- \n\n\tDavid\n"},{"id":"110995","messageId":"20090410081843.GB9369@gmail.com","threadId":"18806","inReplyTo":"20090410074327.GA9369@gmail.com","subject":"Re: [PATCH v2 1/2] Ensure consistent usage of mergetool.keepBackup in git","fromName":"David Aguilar","fromEmail":"davvid@gmail.com","sentAt":"2009-04-10T08:18:44Z","receivedAt":"2009-04-10T08:18:44Z","isPatch":true,"sender":{"key":"davvid@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13196?v=4"},"body":"On 0, David Aguilar wrote:\n> \n> It /seems/ like the docs and completion should be updated.\n\nThough my guess is as good as any....\nI'd rather hear someone else's opinion.\n\n$ git log -p 44c36d1c\ncommit 44c36d1ccc9a40bfb31910dfd7e18d59fa8be502\nAuthor: Charles Bailey \nDate: Thu Feb 21 23:30:02 2008 +0000\n\n Tidy up git mergetool's backup file behaviour\n \n Currently a backup pre-merge file with conflict markers is sometimes\n kept with a .orig extenstion and sometimes removed depending on the\n particular merge tool used.\n \n This patch makes the handling consistent across all merge tools and\n configurable via a new mergetool.keepBackup config variable\n \n Signed-off-by: Charles Bailey \n Signed-off-by: Junio C Hamano \n\n\nThe commit comment says mergetool.keepBackup, even though the code always\nhad it as merge.keepBackup.\n\n$ git log -p 7e30682c\n\nRight now more people have merge.keepbackup already set since git-gui\nhas had it that way for the last 7 months or so. Nevertheless,\nShawn's already applied the git-gui patch which hints that maybe\nwe should just make the code match the docs. In which case, a\npatch against pu would be a good thing, but I would like to\nhear someone else's opinion just so that you don't waste time\ngoing down the wrong route.\n\n-- \n\n\tDavid\n"},{"id":"110996","messageId":"49DF0280.2060000@pelagic.nl","threadId":"18806","inReplyTo":"20090410081843.GB9369@gmail.com","subject":"Re: [PATCH v2 1/2] Ensure consistent usage of mergetool.keepBackup in git","fromName":"Ferry Huberts (Pelagic)","fromEmail":"ferry.huberts@pelagic.nl","sentAt":"2009-04-10T08:25:36Z","receivedAt":"2009-04-10T08:25:36Z","isPatch":true,"sender":{"key":"ferry.huberts@pelagic.nl","avatar":"https://gravatar.com/avatar/9f63c0289ad23cbdef0f7609a0af85ff0f4b3babfd066de9ff58f62d48cfd6f2?d=mp&s=160"},"body":"David Aguilar wrote:\n> On 0, David Aguilar wrote:\n>> It /seems/ like the docs and completion should be updated.\n> \n> Though my guess is as good as any....\n> I'd rather hear someone else's opinion.\n> \n> $ git log -p 44c36d1c\n> commit 44c36d1ccc9a40bfb31910dfd7e18d59fa8be502\n> Author: Charles Bailey \n> Date: Thu Feb 21 23:30:02 2008 +0000\n> \n> Tidy up git mergetool's backup file behaviour\n> \n> Currently a backup pre-merge file with conflict markers is sometimes\n> kept with a .orig extenstion and sometimes removed depending on the\n> particular merge tool used.\n> \n> This patch makes the handling consistent across all merge tools and\n> configurable via a new mergetool.keepBackup config variable\n> \n> Signed-off-by: Charles Bailey \n> Signed-off-by: Junio C Hamano \n> \n> \n> The commit comment says mergetool.keepBackup, even though the code always\n> had it as merge.keepBackup.\n> \n> $ git log -p 7e30682c\n> \n> Right now more people have merge.keepbackup already set since git-gui\n> has had it that way for the last 7 months or so. Nevertheless,\n> Shawn's already applied the git-gui patch which hints that maybe\n> we should just make the code match the docs. In which case, a\n> patch against pu would be a good thing, but I would like to\n> hear someone else's opinion just so that you don't waste time\n> going down the wrong route.\n> \nagree\n"},{"id":"111009","messageId":"200904101648.44593.markus.heidelberg@web.de","threadId":"18806","inReplyTo":"20090410081843.GB9369@gmail.com","subject":"Re: [PATCH v2 1/2] Ensure consistent usage of mergetool.keepBackup in git","fromName":"Markus Heidelberg","fromEmail":"markus.heidelberg@web.de","sentAt":"2009-04-10T14:48:43Z","receivedAt":"2009-04-10T14:48:43Z","isPatch":true,"sender":{"key":"markus.heidelberg@web.de","avatar":"https://avatars.githubusercontent.com/u/6334512?v=4"},"body":"David Aguilar, 10.04.2009:\n> On 0, David Aguilar wrote:\n> > \n> > It /seems/ like the docs and completion should be updated.\n> \n> Though my guess is as good as any....\n> I'd rather hear someone else's opinion.\n\nAnd I've waited for a response from the difftool/mergetool maintainers\nat first, so I waited with my reply till now.\n\n> $ git log -p 44c36d1c\n> commit 44c36d1ccc9a40bfb31910dfd7e18d59fa8be502\n> Author: Charles Bailey \n> Date: Thu Feb 21 23:30:02 2008 +0000\n> \n> Tidy up git mergetool's backup file behaviour\n> \n> Currently a backup pre-merge file with conflict markers is sometimes\n> kept with a .orig extenstion and sometimes removed depending on the\n> particular merge tool used.\n> \n> This patch makes the handling consistent across all merge tools and\n> configurable via a new mergetool.keepBackup config variable\n> \n> Signed-off-by: Charles Bailey \n> Signed-off-by: Junio C Hamano \n> \n> \n> The commit comment says mergetool.keepBackup, even though the code always\n> had it as merge.keepBackup.\n\nYes, merge.keepbackup was never documented, we shouldn't have to keep\nbackwards compatibility for such undocumented behaviour.\n\n> Right now more people have merge.keepbackup already set since git-gui\n> has had it that way for the last 7 months or so.\n\nPeople which noticed that the documented mergetool.keepBackup didn't\nwork, had to look into the source to find out that merge.keepBackup did\nit, so I see no problem with changing it. They rather should have\nreported it anyway.\n\nFerry did it right with fixing this inconsistency.\n\n> Nevertheless,\n> Shawn's already applied the git-gui patch which hints that maybe\n> we should just make the code match the docs.\n\nYes.\n\nMarkus\n"},{"id":"111044","messageId":"20090411120435.GB10381@hashpling.org","threadId":"18806","inReplyTo":"20090410081843.GB9369@gmail.com","subject":"Re: [PATCH v2 1/2] Ensure consistent usage of mergetool.keepBackup in git","fromName":"Charles Bailey","fromEmail":"charles@hashpling.org","sentAt":"2009-04-11T12:04:35Z","receivedAt":"2009-04-11T12:04:35Z","isPatch":true,"sender":{"key":"charles@hashpling.org","avatar":"https://avatars.githubusercontent.com/u/1668475?v=4"},"body":"On Fri, Apr 10, 2009 at 01:18:44AM -0700, David Aguilar wrote:\n> On 0, David Aguilar wrote:\n> > \n> > It /seems/ like the docs and completion should be updated.\n> \n> Though my guess is as good as any....\n> I'd rather hear someone else's opinion.\n> \n> $ git log -p 44c36d1c\n> commit 44c36d1ccc9a40bfb31910dfd7e18d59fa8be502\n> Author: Charles Bailey \n> Date: Thu Feb 21 23:30:02 2008 +0000\n> \n> Tidy up git mergetool's backup file behaviour\n> \n> Currently a backup pre-merge file with conflict markers is sometimes\n> kept with a .orig extenstion and sometimes removed depending on the\n> particular merge tool used.\n> \n> This patch makes the handling consistent across all merge tools and\n> configurable via a new mergetool.keepBackup config variable\n> \n> Signed-off-by: Charles Bailey \n> Signed-off-by: Junio C Hamano \n> \n> \n> The commit comment says mergetool.keepBackup, even though the code always\n> had it as merge.keepBackup.\n\nNot going by the commit message, but rather by the documentation I do\nbelieve that mergetool.keepBackup should have been the correct\nvariable to use. I'm not sure why I missed this at the time.\n\nI seem to remember looking for existing consistency in this before.\nOriginally I was going on the premise that merge.* were things that\nwere general between merges and all mergetools whereas mergetool.*\nwere things that were merge tool specific. Subsequently, it was more\nnatural to have mergetool.$tool.* for things that were tool specifit\nand to have mergetool.* as general mergetool specific settings,\nleaving merge.* as merge specific settings with the exception of\nmerge.tool which is obviously specific to git mergetool, but is in the\nmerge.* section.\n\n-- \nCharles Bailey\nhttp://ccgi.hashpling.plus.com/blog/\n"}]}