{"thread":{"id":"12145","subject":"[PATCH 1/2] Tidy up git mergetool's backup file behaviour and variable names","startedAt":"2008-02-17T12:45:29Z","lastAt":"2008-02-18T08:08:41Z","messageCount":4,"participants":["Charles Bailey","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"68982","messageId":"9543203388d64839de78822efb538903fc15bf7f.1203251306.git.charles@hashpling.org","threadId":"12145","inReplyTo":null,"subject":"[PATCH 1/2] Tidy up git mergetool's backup file behaviour and variable names","fromName":"Charles Bailey","fromEmail":"charles@hashpling.org","sentAt":"2008-02-17T12:45:29Z","receivedAt":"2008-02-17T12:45:29Z","isPatch":true,"sender":{"key":"charles@hashpling.org","avatar":"https://avatars.githubusercontent.com/u/1668475?v=4"},"body":"Currently a backup pre-merge file with conflict markers is sometimes\nkept with a .orig extenstion and sometimes removed depending on the\nparticular merge tool used.\n\nThis patch makes the handling consistent across all merge tools and\nconfigurable via a new mergetool.keepBackup config variable\n\nChanged the merge file path variable to MERGED for consistency with the\nnames of the merge temporary filename variables. This done with the\nintention of having these variables used by user scripts in a subsequent\ncustom merge tool patch.\n\nSigned-off-by: Charles Bailey <charles@hashpling.org>\n---\n\nReordered patch series taking into account Junio's latest comments.\n\n\n Documentation/config.txt |    5 ++\n git-mergetool.sh         |  134 +++++++++++++++++++++-------------------------\n 2 files changed, 67 insertions(+), 72 deletions(-)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex f9bdb16..c145d5b 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -717,6 +717,11 @@ mergetool.<tool>.path::\n \tOverride the path for the given tool.  This is useful in case\n \tyour tool is not in the PATH.\n \n+mergetool.keepBackup::\n+\tAfter performing a merge, the original file with conflict markers\n+\tcan be saved as a file with a `.orig` extension.  If this variable\n+\tis set to `false` then this file is not preserved.\n+\n pack.window::\n \tThe size of the window used by linkgit:git-pack-objects[1] when no\n \twindow size is given on the command line. Defaults to 10.\ndiff --git a/git-mergetool.sh b/git-mergetool.sh\nindex cbbb707..f556927 100755\n--- a/git-mergetool.sh\n+++ b/git-mergetool.sh\n@@ -34,7 +34,7 @@ base_present () {\n \n cleanup_temp_files () {\n     if test \"$1\" = --save-backup ; then\n-\tmv -- \"$BACKUP\" \"$path.orig\"\n+\tmv -- \"$BACKUP\" \"$MERGED.orig\"\n \trm -f -- \"$LOCAL\" \"$REMOTE\" \"$BASE\"\n     else\n \trm -f -- \"$LOCAL\" \"$REMOTE\" \"$BASE\" \"$BACKUP\"\n@@ -67,14 +67,14 @@ resolve_symlink_merge () {\n \tread ans\n \tcase \"$ans\" in\n \t    [lL]*)\n-\t\tgit checkout-index -f --stage=2 -- \"$path\"\n-\t\tgit add -- \"$path\"\n+\t\tgit checkout-index -f --stage=2 -- \"$MERGED\"\n+\t\tgit add -- \"$MERGED\"\n \t\tcleanup_temp_files --save-backup\n \t\treturn\n \t\t;;\n \t    [rR]*)\n-\t\tgit checkout-index -f --stage=3 -- \"$path\"\n-\t\tgit add -- \"$path\"\n+\t\tgit checkout-index -f --stage=3 -- \"$MERGED\"\n+\t\tgit add -- \"$MERGED\"\n \t\tcleanup_temp_files --save-backup\n \t\treturn\n \t\t;;\n@@ -95,12 +95,12 @@ resolve_deleted_merge () {\n \tread ans\n \tcase \"$ans\" in\n \t    [mMcC]*)\n-\t\tgit add -- \"$path\"\n+\t\tgit add -- \"$MERGED\"\n \t\tcleanup_temp_files --save-backup\n \t\treturn\n \t\t;;\n \t    [dD]*)\n-\t\tgit rm -- \"$path\" > /dev/null\n+\t\tgit rm -- \"$MERGED\" > /dev/null\n \t\tcleanup_temp_files\n \t\treturn\n \t\t;;\n@@ -112,11 +112,11 @@ resolve_deleted_merge () {\n }\n \n check_unchanged () {\n-    if test \"$path\" -nt \"$BACKUP\" ; then\n+    if test \"$MERGED\" -nt \"$BACKUP\" ; then\n \tstatus=0;\n     else\n \twhile true; do\n-\t    echo \"$path seems unchanged.\"\n+\t    echo \"$MERGED seems unchanged.\"\n \t    printf \"Was the merge successful? [y/n] \"\n \t    read answer < /dev/tty\n \t    case \"$answer\" in\n@@ -127,50 +127,38 @@ check_unchanged () {\n     fi\n }\n \n-save_backup () {\n-    if test \"$status\" -eq 0; then\n-\tmv -- \"$BACKUP\" \"$path.orig\"\n-    fi\n-}\n-\n-remove_backup () {\n-    if test \"$status\" -eq 0; then\n-\trm \"$BACKUP\"\n-    fi\n-}\n-\n merge_file () {\n-    path=\"$1\"\n+    MERGED=\"$1\"\n \n-    f=`git ls-files -u -- \"$path\"`\n+    f=`git ls-files -u -- \"$MERGED\"`\n     if test -z \"$f\" ; then\n-\tif test ! -f \"$path\" ; then\n-\t    echo \"$path: file not found\"\n+\tif test ! -f \"$MERGED\" ; then\n+\t    echo \"$MERGED: file not found\"\n \telse\n-\t    echo \"$path: file does not need merging\"\n+\t    echo \"$MERGED: file does not need merging\"\n \tfi\n \texit 1\n     fi\n \n-    ext=\"$$$(expr \"$path\" : '.*\\(\\.[^/]*\\)$')\"\n-    BACKUP=\"$path.BACKUP.$ext\"\n-    LOCAL=\"$path.LOCAL.$ext\"\n-    REMOTE=\"$path.REMOTE.$ext\"\n-    BASE=\"$path.BASE.$ext\"\n+    ext=\"$$$(expr \"$MERGED\" : '.*\\(\\.[^/]*\\)$')\"\n+    BACKUP=\"$MERGED.BACKUP.$ext\"\n+    LOCAL=\"$MERGED.LOCAL.$ext\"\n+    REMOTE=\"$MERGED.REMOTE.$ext\"\n+    BASE=\"$MERGED.BASE.$ext\"\n \n-    mv -- \"$path\" \"$BACKUP\"\n-    cp -- \"$BACKUP\" \"$path\"\n+    mv -- \"$MERGED\" \"$BACKUP\"\n+    cp -- \"$BACKUP\" \"$MERGED\"\n \n-    base_mode=`git ls-files -u -- \"$path\" | awk '{if ($3==1) print $1;}'`\n-    local_mode=`git ls-files -u -- \"$path\" | awk '{if ($3==2) print $1;}'`\n-    remote_mode=`git ls-files -u -- \"$path\" | awk '{if ($3==3) print $1;}'`\n+    base_mode=`git ls-files -u -- \"$MERGED\" | awk '{if ($3==1) print $1;}'`\n+    local_mode=`git ls-files -u -- \"$MERGED\" | awk '{if ($3==2) print $1;}'`\n+    remote_mode=`git ls-files -u -- \"$MERGED\" | awk '{if ($3==3) print $1;}'`\n \n-    base_present   && git cat-file blob \":1:$prefix$path\" >\"$BASE\" 2>/dev/null\n-    local_present  && git cat-file blob \":2:$prefix$path\" >\"$LOCAL\" 2>/dev/null\n-    remote_present && git cat-file blob \":3:$prefix$path\" >\"$REMOTE\" 2>/dev/null\n+    base_present   && git cat-file blob \":1:$prefix$MERGED\" >\"$BASE\" 2>/dev/null\n+    local_present  && git cat-file blob \":2:$prefix$MERGED\" >\"$LOCAL\" 2>/dev/null\n+    remote_present && git cat-file blob \":3:$prefix$MERGED\" >\"$REMOTE\" 2>/dev/null\n \n     if test -z \"$local_mode\" -o -z \"$remote_mode\"; then\n-\techo \"Deleted merge conflict for '$path':\"\n+\techo \"Deleted merge conflict for '$MERGED':\"\n \tdescribe_file \"$local_mode\" \"local\" \"$LOCAL\"\n \tdescribe_file \"$remote_mode\" \"remote\" \"$REMOTE\"\n \tresolve_deleted_merge\n@@ -178,14 +166,14 @@ merge_file () {\n     fi\n \n     if is_symlink \"$local_mode\" || is_symlink \"$remote_mode\"; then\n-\techo \"Symbolic link merge conflict for '$path':\"\n+\techo \"Symbolic link merge conflict for '$MERGED':\"\n \tdescribe_file \"$local_mode\" \"local\" \"$LOCAL\"\n \tdescribe_file \"$remote_mode\" \"remote\" \"$REMOTE\"\n \tresolve_symlink_merge\n \treturn\n     fi\n \n-    echo \"Normal merge conflict for '$path':\"\n+    echo \"Normal merge conflict for '$MERGED':\"\n     describe_file \"$local_mode\" \"local\" \"$LOCAL\"\n     describe_file \"$remote_mode\" \"remote\" \"$REMOTE\"\n     printf \"Hit return to start merge resolution tool (%s): \" \"$merge_tool\"\n@@ -194,36 +182,32 @@ merge_file () {\n     case \"$merge_tool\" in\n \tkdiff3)\n \t    if base_present ; then\n-\t\t(\"$merge_tool_path\" --auto --L1 \"$path (Base)\" --L2 \"$path (Local)\" --L3 \"$path (Remote)\" \\\n-\t\t    -o \"$path\" -- \"$BASE\" \"$LOCAL\" \"$REMOTE\" > /dev/null 2>&1)\n+\t\t(\"$merge_tool_path\" --auto --L1 \"$MERGED (Base)\" --L2 \"$MERGED (Local)\" --L3 \"$MERGED (Remote)\" \\\n+\t\t    -o \"$MERGED\" -- \"$BASE\" \"$LOCAL\" \"$REMOTE\" > /dev/null 2>&1)\n \t    else\n-\t\t(\"$merge_tool_path\" --auto --L1 \"$path (Local)\" --L2 \"$path (Remote)\" \\\n-\t\t    -o \"$path\" -- \"$LOCAL\" \"$REMOTE\" > /dev/null 2>&1)\n+\t\t(\"$merge_tool_path\" --auto --L1 \"$MERGED (Local)\" --L2 \"$MERGED (Remote)\" \\\n+\t\t    -o \"$MERGED\" -- \"$LOCAL\" \"$REMOTE\" > /dev/null 2>&1)\n \t    fi\n \t    status=$?\n-\t    remove_backup\n \t    ;;\n \ttkdiff)\n \t    if base_present ; then\n-\t\t\"$merge_tool_path\" -a \"$BASE\" -o \"$path\" -- \"$LOCAL\" \"$REMOTE\"\n+\t\t\"$merge_tool_path\" -a \"$BASE\" -o \"$MERGED\" -- \"$LOCAL\" \"$REMOTE\"\n \t    else\n-\t\t\"$merge_tool_path\" -o \"$path\" -- \"$LOCAL\" \"$REMOTE\"\n+\t\t\"$merge_tool_path\" -o \"$MERGED\" -- \"$LOCAL\" \"$REMOTE\"\n \t    fi\n \t    status=$?\n-\t    save_backup\n \t    ;;\n \tmeld|vimdiff)\n \t    touch \"$BACKUP\"\n-\t    \"$merge_tool_path\" -- \"$LOCAL\" \"$path\" \"$REMOTE\"\n+\t    \"$merge_tool_path\" -- \"$LOCAL\" \"$MERGED\" \"$REMOTE\"\n \t    check_unchanged\n-\t    save_backup\n \t    ;;\n \tgvimdiff)\n-\t\ttouch \"$BACKUP\"\n-\t\t\"$merge_tool_path\" -f -- \"$LOCAL\" \"$path\" \"$REMOTE\"\n-\t\tcheck_unchanged\n-\t\tsave_backup\n-\t\t;;\n+\t    touch \"$BACKUP\"\n+\t    \"$merge_tool_path\" -f -- \"$LOCAL\" \"$MERGED\" \"$REMOTE\"\n+\t    check_unchanged\n+\t    ;;\n \txxdiff)\n \t    touch \"$BACKUP\"\n \t    if base_present ; then\n@@ -231,53 +215,57 @@ merge_file () {\n \t\t    -R 'Accel.SaveAsMerged: \"Ctrl-S\"' \\\n \t\t    -R 'Accel.Search: \"Ctrl+F\"' \\\n \t\t    -R 'Accel.SearchForward: \"Ctrl-G\"' \\\n-\t\t    --merged-file \"$path\" -- \"$LOCAL\" \"$BASE\" \"$REMOTE\"\n+\t\t    --merged-file \"$MERGED\" -- \"$LOCAL\" \"$BASE\" \"$REMOTE\"\n \t    else\n \t\t\"$merge_tool_path\" -X --show-merged-pane \\\n \t\t    -R 'Accel.SaveAsMerged: \"Ctrl-S\"' \\\n \t\t    -R 'Accel.Search: \"Ctrl+F\"' \\\n \t\t    -R 'Accel.SearchForward: \"Ctrl-G\"' \\\n-\t\t    --merged-file \"$path\" -- \"$LOCAL\" \"$REMOTE\"\n+\t\t    --merged-file \"$MERGED\" -- \"$LOCAL\" \"$REMOTE\"\n \t    fi\n \t    check_unchanged\n-\t    save_backup\n \t    ;;\n \topendiff)\n \t    touch \"$BACKUP\"\n \t    if base_present; then\n-\t\t\"$merge_tool_path\" \"$LOCAL\" \"$REMOTE\" -ancestor \"$BASE\" -merge \"$path\" | cat\n+\t\t\"$merge_tool_path\" \"$LOCAL\" \"$REMOTE\" -ancestor \"$BASE\" -merge \"$MERGED\" | cat\n \t    else\n-\t\t\"$merge_tool_path\" \"$LOCAL\" \"$REMOTE\" -merge \"$path\" | cat\n+\t\t\"$merge_tool_path\" \"$LOCAL\" \"$REMOTE\" -merge \"$MERGED\" | cat\n \t    fi\n \t    check_unchanged\n-\t    save_backup\n \t    ;;\n \tecmerge)\n \t    touch \"$BACKUP\"\n \t    if base_present; then\n-\t\t\"$merge_tool_path\" \"$BASE\" \"$LOCAL\" \"$REMOTE\" --mode=merge3 --to=\"$path\"\n+\t\t\"$merge_tool_path\" \"$BASE\" \"$LOCAL\" \"$REMOTE\" --mode=merge3 --to=\"$MERGED\"\n \t    else\n-\t\t\"$merge_tool_path\" \"$LOCAL\" \"$REMOTE\" --mode=merge2 --to=\"$path\"\n+\t\t\"$merge_tool_path\" \"$LOCAL\" \"$REMOTE\" --mode=merge2 --to=\"$MERGED\"\n \t    fi\n \t    check_unchanged\n-\t    save_backup\n \t    ;;\n \temerge)\n \t    if base_present ; then\n-\t\t\"$merge_tool_path\" -f emerge-files-with-ancestor-command \"$LOCAL\" \"$REMOTE\" \"$BASE\" \"$(basename \"$path\")\"\n+\t\t\"$merge_tool_path\" -f emerge-files-with-ancestor-command \"$LOCAL\" \"$REMOTE\" \"$BASE\" \"$(basename \"$MERGED\")\"\n \t    else\n-\t\t\"$merge_tool_path\" -f emerge-files-command \"$LOCAL\" \"$REMOTE\" \"$(basename \"$path\")\"\n+\t\t\"$merge_tool_path\" -f emerge-files-command \"$LOCAL\" \"$REMOTE\" \"$(basename \"$MERGED\")\"\n \t    fi\n \t    status=$?\n-\t    save_backup\n \t    ;;\n     esac\n+\n     if test \"$status\" -ne 0; then\n-\techo \"merge of $path failed\" 1>&2\n-\tmv -- \"$BACKUP\" \"$path\"\n+\techo \"merge of $MERGED failed\" 1>&2\n+\tmv -- \"$BACKUP\" \"$MERGED\"\n \texit 1\n     fi\n-    git add -- \"$path\"\n+\n+    if test \"$merge_keep_backup\" = \"true\"; then\n+\tmv -- \"$BACKUP\" \"$MERGED.orig\"\n+    else\n+\trm -- \"$BACKUP\"\n+    fi\n+\n+    git add -- \"$MERGED\"\n     cleanup_temp_files\n }\n \n@@ -380,6 +368,8 @@ else\n \n     init_merge_tool_path \"$merge_tool\"\n \n+    merge_keep_backup=\"$(git config --bool merge.keepBackup || echo true)\"\n+\n     if ! type \"$merge_tool_path\" > /dev/null 2>&1; then\n         echo \"The merge tool $merge_tool is not available as '$merge_tool_path'\"\n         exit 1\n-- \n1.5.4.1.34.g94bf\n"},{"id":"68983","messageId":"6efa9ecd7fe0b8c180652fbe7c03d3c63f203259.1203251306.git.charles@hashpling.org","threadId":"12145","inReplyTo":"9543203388d64839de78822efb538903fc15bf7f.1203251306.git.charles@hashpling.org","subject":"[PATCH 2/2] Teach git mergetool to use custom commands defined at config time","fromName":"Charles Bailey","fromEmail":"charles@hashpling.org","sentAt":"2008-02-17T12:46:26Z","receivedAt":"2008-02-17T12:46:26Z","isPatch":true,"sender":{"key":"charles@hashpling.org","avatar":"https://avatars.githubusercontent.com/u/1668475?v=4"},"body":"Currently git mergetool is restricted to a set of commands defined\nin the script. You can subvert the mergetool.<tool>.path to force\ngit mergetool to use a different command, but if you have a command\nwhose invocation syntax does not match one of the current tools then\nyou would have to write a wrapper script for it.\n\nThis patch adds three git config variable patterns which allow a more\nflexible choice of merge tool.\n\nIf you run git mergetool with -t/--tool or the merge.tool config\nvariable set to an unrecognized tool then git mergetool will query the\nmergetool.<tool>.cmd config variable. If this variable exists, then\ngit mergetool will treat the specified tool as a custom command and\nwill use a shell eval to run the command so that the appropriate shell\nvariables can be used to find the merge temporary files.\n\nmergetool.<tool>.trustExitCode can be used to indicate that the exit\ncode of the custom command can be used to determine the success of the\nmerge. mergetool.<tool>.keepBackup can be used to specify whether the\noriginal pre-merge file with conflict markers should be kept as a\n'.orig' file after the merge tool completes.\n\nSigned-off-by: Charles Bailey <charles@hashpling.org>\n---\n Documentation/config.txt |   25 +++++++++++++++++++++++--\n git-mergetool.sh         |   29 +++++++++++++++++++++++++++--\n 2 files changed, 50 insertions(+), 4 deletions(-)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex c145d5b..60aa0d7 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -689,8 +689,10 @@ merge.summary::\n \n merge.tool::\n \tControls which merge resolution program is used by\n-\tlinkgit:git-mergetool[1].  Valid values are: \"kdiff3\", \"tkdiff\",\n-\t\"meld\", \"xxdiff\", \"emerge\", \"vimdiff\", \"gvimdiff\", and \"opendiff\".\n+\tlinkgit:git-mergetool[1].  Valid built-in values are: \"kdiff3\",\n+\t\"tkdiff\", \"meld\", \"xxdiff\", \"emerge\", \"vimdiff\", \"gvimdiff\", and\n+\t\"opendiff\".  Any other value is treated is custom merge tool\n+\tand there must be a corresponing mergetool.<tool>.cmd option.\n \n merge.verbosity::\n \tControls the amount of output shown by the recursive merge\n@@ -717,6 +719,25 @@ mergetool.<tool>.path::\n \tOverride the path for the given tool.  This is useful in case\n \tyour tool is not in the PATH.\n \n+mergetool.<tool>.cmd::\n+\tSpecify the command to invoke the specified merge tool.  The\n+\tspecified command is evaluated in shell with the following\n+\tvariables available: 'BASE' is the name of a temporary file\n+\tcontaining the common base of the files to be merged, if available;\n+\t'LOCAL' is the name of a temporary file containing the contents of\n+\tthe file on the current branch; 'REMOTE' is the name of a temporary\n+\tfile containing the contents of the file from the branch being\n+\tmerged; 'MERGED' contains the name of the file to which the merge\n+\ttool should write the results of a successful merge.\n+\n+mergetool.<tool>.trustExitCode::\n+\tFor a custom merge command, specify whether the exit code of\n+\tthe merge command can be used to determine whether the merge was\n+\tsuccessful.  If this is not set to true then the merge target file\n+\ttimestamp is checked and the merge assumed to have been successful\n+\tif the file has been updated, otherwise the user is prompted to\n+\tindicate the success of the merge.\n+\n mergetool.keepBackup::\n \tAfter performing a merge, the original file with conflict markers\n \tcan be saved as a file with a `.orig` extension.  If this variable\ndiff --git a/git-mergetool.sh b/git-mergetool.sh\nindex f556927..fd0812b 100755\n--- a/git-mergetool.sh\n+++ b/git-mergetool.sh\n@@ -251,6 +251,19 @@ merge_file () {\n \t    fi\n \t    status=$?\n \t    ;;\n+\t*)\n+\t    if test -n \"$merge_tool_cmd\"; then\n+\t\tif test \"$merge_tool_trust_exit_code\" = \"false\"; then\n+\t\t    touch \"$BACKUP\"\n+\t\t    ( eval $merge_tool_cmd )\n+\t\t    check_unchanged\n+\t\telse\n+\t\t    ( eval $merge_tool_cmd )\n+\t\t    status=$?\n+\t\tfi\n+\t\tsave_backup\n+\t    fi\n+\t    ;;\n     esac\n \n     if test \"$status\" -ne 0; then\n@@ -297,12 +310,20 @@ do\n     shift\n done\n \n+valid_custom_tool()\n+{\n+    merge_tool_cmd=\"$(git config mergetool.$1.cmd)\"\n+    test -n \"$merge_tool_cmd\"\n+}\n+\n valid_tool() {\n \tcase \"$1\" in\n \t\tkdiff3 | tkdiff | xxdiff | meld | opendiff | emerge | vimdiff | gvimdiff | ecmerge)\n \t\t\t;; # happy\n \t\t*)\n-\t\t\treturn 1\n+\t\t\tif ! valid_custom_tool \"$1\"; then\n+\t\t\t\treturn 1\n+\t\t\tfi\n \t\t\t;;\n \tesac\n }\n@@ -370,10 +391,14 @@ else\n \n     merge_keep_backup=\"$(git config --bool merge.keepBackup || echo true)\"\n \n-    if ! type \"$merge_tool_path\" > /dev/null 2>&1; then\n+    if test -z \"$merge_tool_cmd\" && ! type \"$merge_tool_path\" > /dev/null 2>&1; then\n         echo \"The merge tool $merge_tool is not available as '$merge_tool_path'\"\n         exit 1\n     fi\n+\n+    if ! test -z \"$merge_tool_cmd\"; then\n+        merge_tool_trust_exit_code=\"$(git config --bool mergetool.$merge_tool.trustExitCode || echo false)\"\n+    fi\n fi\n \n \n-- \n1.5.4.1.34.g94bf\n\n\n-- \nCharles Bailey\nhttp://ccgi.hashpling.plus.com/blog/\n"},{"id":"69084","messageId":"7vy79jsza1.fsf@gitster.siamese.dyndns.org","threadId":"12145","inReplyTo":"9543203388d64839de78822efb538903fc15bf7f.1203251306.git.charles@hashpling.org","subject":"Re: [PATCH 1/2] Tidy up git mergetool's backup file behaviour and variable names","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-02-18T03:45:42Z","receivedAt":"2008-02-18T03:45:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Charles Bailey <charles@hashpling.org> writes:\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> Changed the merge file path variable to MERGED for consistency with the\n> names of the merge temporary filename variables. This done with the\n> intention of having these variables used by user scripts in a subsequent\n> custom merge tool patch.\n\nI would have preferred two separate patches, one s/path/MERGED/\nand the other save/remove clean-up, which would be much easier\nway to review, but this is what we have, so let's work on this\nversion.\n\n> +mergetool.keepBackup::\n> +\tAfter performing a merge, the original file with conflict markers\n> +\tcan be saved as a file with a `.orig` extension.  If this variable\n> +\tis set to `false` then this file is not preserved.\n> +\n\ns/$/  Defaults to true (i.e. keep the backup files)/.\n\nWe might also want a command line option to override the user's\nusual default specified with this configuration but that can be\nleft for later rounds.\n\n> @@ -112,11 +112,11 @@ resolve_deleted_merge () {\n>  }\n>  \n>  check_unchanged () {\n> -    if test \"$path\" -nt \"$BACKUP\" ; then\n> +    if test \"$MERGED\" -nt \"$BACKUP\" ; then\n\nI think this is the cause of your automated test sometimes\nneeding 1 sec sleep.  This check should perhaps be done with a\n\"!  cmp $MERGED $BACKUP >/dev/null\", which would succeed if the\nuser's interaction with the backend tool touched the file.\n\nThe rest of the patch looked fine to me.\n\nWe might also want to add -y (assume \"Yes\" answer to \"Did you\nresolve it\" question without actually asking) command line\noption to the script, but that would be for later rounds.\n"},{"id":"69097","messageId":"20080218080841.GA12008@hashpling.org","threadId":"12145","inReplyTo":"7vy79jsza1.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH 1/2] Tidy up git mergetool's backup file behaviour and variable names","fromName":"Charles Bailey","fromEmail":"charles@hashpling.org","sentAt":"2008-02-18T08:08:41Z","receivedAt":"2008-02-18T08:08:41Z","isPatch":true,"sender":{"key":"charles@hashpling.org","avatar":"https://avatars.githubusercontent.com/u/1668475?v=4"},"body":"On Sun, Feb 17, 2008 at 07:45:42PM -0800, Junio C Hamano wrote:\n> Charles Bailey <charles@hashpling.org> writes:\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> > Changed the merge file path variable to MERGED for consistency with the\n> > names of the merge temporary filename variables. This done with the\n> > intention of having these variables used by user scripts in a subsequent\n> > custom merge tool patch.\n> \n> I would have preferred two separate patches, one s/path/MERGED/\n> and the other save/remove clean-up, which would be much easier\n> way to review, but this is what we have, so let's work on this\n> version.\n\nFair enough, I'll send an update.\n> \n> > +mergetool.keepBackup::\n> > +\tAfter performing a merge, the original file with conflict markers\n> > +\tcan be saved as a file with a `.orig` extension.  If this variable\n> > +\tis set to `false` then this file is not preserved.\n> > +\n> \n> s/$/  Defaults to true (i.e. keep the backup files)/.\n\nNoted.\n\n> We might also want a command line option to override the user's\n> usual default specified with this configuration but that can be\n> left for later rounds.\n\nAgreed, I'll tackle this later, given time.\n\n> > @@ -112,11 +112,11 @@ resolve_deleted_merge () {\n> >  }\n> >  \n> >  check_unchanged () {\n> > -    if test \"$path\" -nt \"$BACKUP\" ; then\n> > +    if test \"$MERGED\" -nt \"$BACKUP\" ; then\n> \n> I think this is the cause of your automated test sometimes\n> needing 1 sec sleep.  This check should perhaps be done with a\n> \"!  cmp $MERGED $BACKUP >/dev/null\", which would succeed if the\n> user's interaction with the backend tool touched the file.\n\nI don't think that we should spend too much time on this seeing as I\nnow fail to reproduce it, but as the code was avoiding the interactive\npath (i.e. trusting the exit code), it shouldn't have been this check.\n\n> \n> The rest of the patch looked fine to me.\n> \n> We might also want to add -y (assume \"Yes\" answer to \"Did you\n> resolve it\" question without actually asking) command line\n> option to the script, but that would be for later rounds.\n\nYes, and this would help the automated tests.\n"}]}