{"thread":{"id":"32715","subject":"[PATCH] mergetool: clean up temp files when aborted","startedAt":"2013-01-24T18:23:49Z","lastAt":"2013-01-24T21:41:57Z","messageCount":4,"participants":["Phil Hord","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"207700","messageId":"1359051829-21331-1-git-send-email-hordp@cisco.com","threadId":"32715","inReplyTo":null,"subject":"[PATCH] mergetool: clean up temp files when aborted","fromName":"Phil Hord","fromEmail":"hordp@cisco.com","sentAt":"2013-01-24T18:23:49Z","receivedAt":"2013-01-24T18:23:49Z","isPatch":true,"sender":{"key":"phil.hord@gmail.com","avatar":"https://avatars.githubusercontent.com/u/123908?v=4"},"body":"When handling a symlink conflict or a deleted-file conflict, mergetool\nstops to ask the user what to do. If the user chooses any option besides\n\"(a)bort\", then the temporary files which mergetool created in\npreparation for handling the conflict are removed. But these temporary\nfiles are not removed when the user chooses to abort the operation.\n\n $ git cherry-pick other/branch\n error: could not apply 4e43581... Fix foo.c\n\n $ git status --short\n DU foo.c\n\n $ git mergetool\n Merging:\n foo.c\n\n Deleted merge conflict for 'foo.c':\n {local}: deleted\n {remote}: modified file\n Use (m)odified or (d)eleted file, or (a)bort? a\n Continue merging other unresolved paths (y/n) ? n\n\n $ git status --short\n DU foo.c\n ?? foo.c.BACKUP.16929.c\n ?? foo.c.BASE.16929.c\n ?? foo.c.LOCAL.16929.c\n ?? foo.c.REMOTE.16929.c\n\nThese temporary files should not remain after the mergetool operation is\ncompleted.\n\nRemove the temporary files by calling the cleanup_temp_files when the\nuser chooses to abort the mergetool operation.\n\nIt looks like 'cleanup_temp_files' without the --save-backups option is\nthe correct thing to do, and this is how this commit is implemented. But\nsome other paths do use --save-backups resulting in a foo.c.orig file\nbeing left behind. That seems to be a different bug, though.\n\nSigned-off-by: Phil Hord \n---\n git-mergetool.sh | 2 ++\n 1 file changed, 2 insertions(+)\n\ndiff --git a/git-mergetool.sh b/git-mergetool.sh\nindex c50e18a..bb93b70 100755\n--- a/git-mergetool.sh\n+++ b/git-mergetool.sh\n@@ -90,6 +90,7 @@ resolve_symlink_merge () {\n \t\t\treturn 0\n \t\t\t;;\n \t\t[aA]*)\n+\t\t\tcleanup_temp_files\n \t\t\treturn 1\n \t\t\t;;\n \t\tesac\n@@ -118,6 +119,7 @@ resolve_deleted_merge () {\n \t\t\treturn 0\n \t\t\t;;\n \t\t[aA]*)\n+\t\t\tcleanup_temp_files\n \t\t\treturn 1\n \t\t\t;;\n \t\tesac\n-- \n1.8.1.1.ga649ac9.dirty\n"},{"id":"207706","messageId":"7vbocebblo.fsf@alter.siamese.dyndns.org","threadId":"32715","inReplyTo":"1359051829-21331-1-git-send-email-hordp@cisco.com","subject":"Re: [PATCH] mergetool: clean up temp files when aborted","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-01-24T19:54:43Z","receivedAt":"2013-01-24T19:54:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Phil Hord writes:\n\n> When handling a symlink conflict or a deleted-file conflict, mergetool\n> stops to ask the user what to do. If the user chooses any option besides\n> \"(a)bort\", then the temporary files which mergetool created in\n> preparation for handling the conflict are removed. But these temporary\n> files are not removed when the user chooses to abort the operation.\n>\n> $ git cherry-pick other/branch\n> error: could not apply 4e43581... Fix foo.c\n>\n> $ git status --short\n> DU foo.c\n>\n> $ git mergetool\n> Merging:\n> foo.c\n>\n> Deleted merge conflict for 'foo.c':\n> {local}: deleted\n> {remote}: modified file\n> Use (m)odified or (d)eleted file, or (a)bort? a\n> Continue merging other unresolved paths (y/n) ? n\n>\n> $ git status --short\n> DU foo.c\n> ?? foo.c.BACKUP.16929.c\n> ?? foo.c.BASE.16929.c\n> ?? foo.c.LOCAL.16929.c\n> ?? foo.c.REMOTE.16929.c\n>\n> These temporary files should not remain after the mergetool operation is\n> completed.\n\nAren't there cases where people \"abort\" so that they can have a\nchance inspect them outside mergetool program? If there are no such\ncases, then I would agree with your claim \"should not remain\", but\nthe proposed log message does not explay why it is sure that there\nare no such use cases.\n\n>\n> Remove the temporary files by calling the cleanup_temp_files when the\n> user chooses to abort the mergetool operation.\n>\n> It looks like 'cleanup_temp_files' without the --save-backups option is\n> the correct thing to do, and this is how this commit is implemented. But\n> some other paths do use --save-backups resulting in a foo.c.orig file\n> being left behind. That seems to be a different bug, though.\n>\n> Signed-off-by: Phil Hord \n> ---\n> git-mergetool.sh | 2 ++\n> 1 file changed, 2 insertions(+)\n>\n> diff --git a/git-mergetool.sh b/git-mergetool.sh\n> index c50e18a..bb93b70 100755\n> --- a/git-mergetool.sh\n> +++ b/git-mergetool.sh\n> @@ -90,6 +90,7 @@ resolve_symlink_merge () {\n> \t\t\treturn 0\n> \t\t\t;;\n> \t\t[aA]*)\n> +\t\t\tcleanup_temp_files\n> \t\t\treturn 1\n> \t\t\t;;\n> \t\tesac\n> @@ -118,6 +119,7 @@ resolve_deleted_merge () {\n> \t\t\treturn 0\n> \t\t\t;;\n> \t\t[aA]*)\n> +\t\t\tcleanup_temp_files\n> \t\t\treturn 1\n> \t\t\t;;\n> \t\tesac\n"},{"id":"207722","messageId":"CABURp0qxj0HVHbYOR1Enfx_A1-ALPDGhLriGp=gab-LBigwt8g@mail.gmail.com","threadId":"32715","inReplyTo":"7vbocebblo.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] mergetool: clean up temp files when aborted","fromName":"Phil Hord","fromEmail":"phil.hord@gmail.com","sentAt":"2013-01-24T21:16:27Z","receivedAt":"2013-01-24T21:16:27Z","isPatch":true,"sender":{"key":"phil.hord@gmail.com","avatar":"https://avatars.githubusercontent.com/u/123908?v=4"},"body":"On Thu, Jan 24, 2013 at 2:54 PM, Junio C Hamano wrote:\n> Phil Hord writes:\n>\n>> When handling a symlink conflict or a deleted-file conflict, mergetool\n>> stops to ask the user what to do. If the user chooses any option besides\n>> \"(a)bort\", then the temporary files which mergetool created in\n>> preparation for handling the conflict are removed. But these temporary\n>> files are not removed when the user chooses to abort the operation.\n>>\n>> $ git cherry-pick other/branch\n>> error: could not apply 4e43581... Fix foo.c\n>>\n>> $ git status --short\n>> DU foo.c\n>>\n>> $ git mergetool\n>> Merging:\n>> foo.c\n>>\n>> Deleted merge conflict for 'foo.c':\n>> {local}: deleted\n>> {remote}: modified file\n>> Use (m)odified or (d)eleted file, or (a)bort? a\n>> Continue merging other unresolved paths (y/n) ? n\n>>\n>> $ git status --short\n>> DU foo.c\n>> ?? foo.c.BACKUP.16929.c\n>> ?? foo.c.BASE.16929.c\n>> ?? foo.c.LOCAL.16929.c\n>> ?? foo.c.REMOTE.16929.c\n>>\n>> These temporary files should not remain after the mergetool operation is\n>> completed.\n>\n> Aren't there cases where people \"abort\" so that they can have a\n> chance inspect them outside mergetool program? If there are no such\n> cases, then I would agree with your claim \"should not remain\", but\n> the proposed log message does not explay why it is sure that there\n> are no such use cases.\n\nYou may be right about other people's workflows which I forgot to\nconsider. I have noticed a lot of inconsistency in this tool wrt to\nmergetool.keepBackup and mergetool.keepTemporaries. Perhaps this\nchange should hinge on the latter.\n\nWould you agree with this rephrased statement (accompanied by matching logic)?\n\n When mergetool.keepTemporaries is false or not set, these temporary files\n should not remain when the mergetool operation is aborted.\n\n\nHere is the wording from git help config:\n\n mergetool.keepBackup\n After performing a merge, the original file with conflict\nmarkers can be saved as a file with a .orig extension. If this\nvariable is set to false then this file\n is not preserved. Defaults to true (i.e. keep the backup files).\n\n mergetool.keepTemporaries\n When invoking a custom merge tool, git uses a set of\ntemporary files to pass to the tool. If the tool returns an error and\nthis variable is set to true, then\n these temporary files will be preserved, otherwise they\nwill be removed after the tool has exited. Defaults to false.\n\n\nI have another commit in the works to clean up consistency around the\nkeepBackup. I'll re-roll this one at the same time.\n\nPhil\n"},{"id":"207723","messageId":"7vy5fi8di2.fsf@alter.siamese.dyndns.org","threadId":"32715","inReplyTo":"CABURp0qxj0HVHbYOR1Enfx_A1-ALPDGhLriGp=gab-LBigwt8g@mail.gmail.com","subject":"Re: [PATCH] mergetool: clean up temp files when aborted","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-01-24T21:41:57Z","receivedAt":"2013-01-24T21:41:57Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Phil Hord writes:\n\n> On Thu, Jan 24, 2013 at 2:54 PM, Junio C Hamano wrote:\n>> Phil Hord writes:\n>> ...\n>>> These temporary files should not remain after the mergetool operation is\n>>> completed.\n>>\n>> Aren't there cases where people \"abort\" so that they can have a\n>> chance inspect them outside mergetool program? If there are no such\n>> cases, then I would agree with your claim \"should not remain\", but\n>> the proposed log message does not explay why it is sure that there\n>> are no such use cases.\n>\n> You may be right about other people's workflows which I forgot to\n> consider. I have noticed a lot of inconsistency in this tool wrt to\n> mergetool.keepBackup and mergetool.keepTemporaries. Perhaps this\n> change should hinge on the latter.\n>\n> Would you agree with this rephrased statement (accompanied by matching logic)?\n>\n> When mergetool.keepTemporaries is false or not set, these temporary files\n> should not remain when the mergetool operation is aborted.\n\nThat is perfectly sensible (and should apply equally to non-\"abort\"\ncases, I think).\n\nThanks.\n"}]}