{"thread":{"id":"38021","subject":"[PATCH 0/3] mergetool/difftool cleanup","startedAt":"2014-11-21T01:20:26Z","lastAt":"2014-11-22T00:20:00Z","messageCount":8,"participants":["David Aguilar","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"252316","messageId":"1416532829-68662-1-git-send-email-davvid@gmail.com","threadId":"38021","inReplyTo":null,"subject":"[PATCH 0/3] mergetool/difftool cleanup","fromName":"David Aguilar","fromEmail":"davvid@gmail.com","sentAt":"2014-11-21T01:20:26Z","receivedAt":"2014-11-21T01:20:26Z","isPatch":true,"sender":{"key":"davvid@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13196?v=4"},"body":"This is a cleanup series to remove the use of the $status\nglobal variable in mergetool/difftool.\n\nThis should wait until after the current RC series is\nover but figured I'd send it out.\n\nDavid Aguilar (3):\n  mergetool--lib: remove use of $status global\n  difftool--helper: add explicit exit statement\n  mergetool: simplify conditionals\n\n git-difftool--helper.sh |  2 ++\n git-mergetool--lib.sh   | 20 +++++---------------\n git-mergetool.sh        | 16 +++++-----------\n 3 files changed, 12 insertions(+), 26 deletions(-)\n\n-- \n2.2.0.rc2.26.g3e3388f\n"},{"id":"252317","messageId":"1416532829-68662-2-git-send-email-davvid@gmail.com","threadId":"38021","inReplyTo":"1416532829-68662-1-git-send-email-davvid@gmail.com","subject":"[PATCH 1/3] mergetool--lib: remove use of $status global","fromName":"David Aguilar","fromEmail":"davvid@gmail.com","sentAt":"2014-11-21T01:20:27Z","receivedAt":"2014-11-21T01:20:27Z","isPatch":true,"sender":{"key":"davvid@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13196?v=4"},"body":"Remove return statements and rework check_unchanged() so that the exit\nstatus from the last evaluated expression bubbles up to the callers.\n\nSigned-off-by: David Aguilar <davvid@gmail.com>\n---\n git-mergetool--lib.sh | 20 +++++---------------\n 1 file changed, 5 insertions(+), 15 deletions(-)\n\ndiff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh\nindex 2b66351..fe61e89 100644\n--- a/git-mergetool--lib.sh\n+++ b/git-mergetool--lib.sh\n@@ -92,7 +92,7 @@ translate_merge_tool_path () {\n check_unchanged () {\n \tif test \"$MERGED\" -nt \"$BACKUP\"\n \tthen\n-\t\tstatus=0\n+\t\treturn 0\n \telse\n \t\twhile true\n \t\tdo\n@@ -100,8 +100,8 @@ check_unchanged () {\n \t\t\tprintf \"Was the merge successful? [y/n] \"\n \t\t\tread answer || return 1\n \t\t\tcase \"$answer\" in\n-\t\t\ty*|Y*) status=0; break ;;\n-\t\t\tn*|N*) status=1; break ;;\n+\t\t\ty*|Y*) return 0 ;;\n+\t\t\tn*|N*) return 1 ;;\n \t\t\tesac\n \t\tdone\n \tfi\n@@ -119,8 +119,6 @@ setup_user_tool () {\n \n \tdiff_cmd () {\n \t\t( eval $merge_tool_cmd )\n-\t\tstatus=$?\n-\t\treturn $status\n \t}\n \n \tmerge_cmd () {\n@@ -130,13 +128,10 @@ setup_user_tool () {\n \t\tthen\n \t\t\ttouch \"$BACKUP\"\n \t\t\t( eval $merge_tool_cmd )\n-\t\t\tstatus=$?\n \t\t\tcheck_unchanged\n \t\telse\n \t\t\t( eval $merge_tool_cmd )\n-\t\t\tstatus=$?\n \t\tfi\n-\t\treturn $status\n \t}\n }\n \n@@ -153,13 +148,11 @@ setup_tool () {\n \t}\n \n \tdiff_cmd () {\n-\t\tstatus=1\n-\t\treturn $status\n+\t\treturn 1\n \t}\n \n \tmerge_cmd () {\n-\t\tstatus=1\n-\t\treturn $status\n+\t\treturn 1\n \t}\n \n \ttranslate_merge_tool_path () {\n@@ -210,7 +203,6 @@ run_merge_tool () {\n \n \tmerge_tool_path=$(get_merge_tool_path \"$1\") || exit\n \tbase_present=\"$2\"\n-\tstatus=0\n \n \t# Bring tool-specific functions into scope\n \tsetup_tool \"$1\" || return 1\n@@ -221,8 +213,6 @@ run_merge_tool () {\n \telse\n \t\trun_diff_cmd \"$1\"\n \tfi\n-\tstatus=$?\n-\treturn $status\n }\n \n # Run a either a configured or built-in diff tool\n-- \n2.2.0.rc2.26.g3e3388f\n"},{"id":"252318","messageId":"1416532829-68662-3-git-send-email-davvid@gmail.com","threadId":"38021","inReplyTo":"1416532829-68662-1-git-send-email-davvid@gmail.com","subject":"[PATCH 2/3] difftool--helper: add explicit exit statement","fromName":"David Aguilar","fromEmail":"davvid@gmail.com","sentAt":"2014-11-21T01:20:28Z","receivedAt":"2014-11-21T01:20:28Z","isPatch":true,"sender":{"key":"davvid@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13196?v=4"},"body":"git-difftool--helper returns a zero exit status unless\n--trust-exit-code is in effect.  Add an explicit exit statement\nto make this clearer.\n\nSigned-off-by: David Aguilar <davvid@gmail.com>\n---\n git-difftool--helper.sh | 2 ++\n 1 file changed, 2 insertions(+)\n\ndiff --git a/git-difftool--helper.sh b/git-difftool--helper.sh\nindex d4fb6df..2b11b1d 100755\n--- a/git-difftool--helper.sh\n+++ b/git-difftool--helper.sh\n@@ -94,3 +94,5 @@ else\n \t\tshift 7\n \tdone\n fi\n+\n+exit 0\n-- \n2.2.0.rc2.26.g3e3388f\n"},{"id":"252319","messageId":"1416532829-68662-4-git-send-email-davvid@gmail.com","threadId":"38021","inReplyTo":"1416532829-68662-1-git-send-email-davvid@gmail.com","subject":"[PATCH 3/3] mergetool: simplify conditionals","fromName":"David Aguilar","fromEmail":"davvid@gmail.com","sentAt":"2014-11-21T01:20:29Z","receivedAt":"2014-11-21T01:20:29Z","isPatch":true,"sender":{"key":"davvid@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13196?v=4"},"body":"Combine the $last_status checks into a single conditional.\nReplace $last_status and $rollup_status with a single variable.\n\nSigned-off-by: David Aguilar <davvid@gmail.com>\n---\n git-mergetool.sh | 16 +++++-----------\n 1 file changed, 5 insertions(+), 11 deletions(-)\n\ndiff --git a/git-mergetool.sh b/git-mergetool.sh\nindex ff050e5..d20581c 100755\n--- a/git-mergetool.sh\n+++ b/git-mergetool.sh\n@@ -426,8 +426,6 @@ fi\n merge_keep_backup=\"$(git config --bool mergetool.keepBackup || echo true)\"\n merge_keep_temporaries=\"$(git config --bool mergetool.keepTemporaries || echo false)\"\n \n-last_status=0\n-rollup_status=0\n files=\n \n if test $# -eq 0\n@@ -455,19 +453,15 @@ printf \"%s\\n\" \"$files\"\n \n IFS='\n '\n+rc=0\n for i in $files\n do\n-\tif test $last_status -ne 0\n-\tthen\n-\t\tprompt_after_failed_merge || exit 1\n-\tfi\n \tprintf \"\\n\"\n-\tmerge_file \"$i\"\n-\tlast_status=$?\n-\tif test $last_status -ne 0\n+\tif ! merge_file \"$i\"\n \tthen\n-\t\trollup_status=1\n+\t\trc=1\n+\t\tprompt_after_failed_merge || exit 1\n \tfi\n done\n \n-exit $rollup_status\n+exit $rc\n-- \n2.2.0.rc2.26.g3e3388f\n"},{"id":"252322","messageId":"1416560590-26088-1-git-send-email-davvid@gmail.com","threadId":"38021","inReplyTo":"1416532829-68662-4-git-send-email-davvid@gmail.com","subject":"[PATCH] mergetools: stop setting $status in merge_cmd()","fromName":"David Aguilar","fromEmail":"davvid@gmail.com","sentAt":"2014-11-21T09:03:10Z","receivedAt":"2014-11-21T09:03:10Z","isPatch":true,"sender":{"key":"davvid@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13196?v=4"},"body":"No callers rely on $status so there's don't need to set\nit during merge_cmd() for diffmerge, emerge, and kdiff3.\n\nSigned-off-by: David Aguilar <davvid@gmail.com>\n---\nThis is based on the mergetool/difftool cleanup patches.\n\n mergetools/diffmerge | 1 -\n mergetools/emerge    | 1 -\n mergetools/kdiff3    | 1 -\n 3 files changed, 3 deletions(-)\n\ndiff --git a/mergetools/diffmerge b/mergetools/diffmerge\nindex 85ac720..f138cb4 100644\n--- a/mergetools/diffmerge\n+++ b/mergetools/diffmerge\n@@ -11,5 +11,4 @@ merge_cmd () {\n \t\t\"$merge_tool_path\" --merge \\\n \t\t\t--result=\"$MERGED\" \"$LOCAL\" \"$REMOTE\"\n \tfi\n-\tstatus=$?\n }\ndiff --git a/mergetools/emerge b/mergetools/emerge\nindex f96d9e5..7b895fd 100644\n--- a/mergetools/emerge\n+++ b/mergetools/emerge\n@@ -15,7 +15,6 @@ merge_cmd () {\n \t\t\t\"$LOCAL\" \"$REMOTE\" \\\n \t\t\t\"$(basename \"$MERGED\")\"\n \tfi\n-\tstatus=$?\n }\n \n translate_merge_tool_path() {\ndiff --git a/mergetools/kdiff3 b/mergetools/kdiff3\nindex a30034f..793d129 100644\n--- a/mergetools/kdiff3\n+++ b/mergetools/kdiff3\n@@ -20,5 +20,4 @@ merge_cmd () {\n \t\t\t-o \"$MERGED\" \"$LOCAL\" \"$REMOTE\" \\\n \t\t>/dev/null 2>&1\n \tfi\n-\tstatus=$?\n }\n-- \n2.2.0.rc2.26.g6d3471d.dirty\n"},{"id":"252358","messageId":"xmqqsihcz7mm.fsf@gitster.dls.corp.google.com","threadId":"38021","inReplyTo":"1416532829-68662-2-git-send-email-davvid@gmail.com","subject":"Re: [PATCH 1/3] mergetool--lib: remove use of $status global","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-11-21T19:16:49Z","receivedAt":"2014-11-21T19:16:49Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"David Aguilar <davvid@gmail.com> writes:\n\n> Remove return statements and rework check_unchanged() so that the exit\n> status from the last evaluated expression bubbles up to the callers.\n>\n> Signed-off-by: David Aguilar <davvid@gmail.com>\n> ---\n>  git-mergetool--lib.sh | 20 +++++---------------\n>  1 file changed, 5 insertions(+), 15 deletions(-)\n>\n> diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh\n> index 2b66351..fe61e89 100644\n> --- a/git-mergetool--lib.sh\n> +++ b/git-mergetool--lib.sh\n> @@ -92,7 +92,7 @@ translate_merge_tool_path () {\n>  check_unchanged () {\n>  \tif test \"$MERGED\" -nt \"$BACKUP\"\n>  \tthen\n> -\t\tstatus=0\n> +\t\treturn 0\n>  \telse\n>  \t\twhile true\n>  \t\tdo\n> @@ -100,8 +100,8 @@ check_unchanged () {\n>  \t\t\tprintf \"Was the merge successful? [y/n] \"\n>  \t\t\tread answer || return 1\n>  \t\t\tcase \"$answer\" in\n> -\t\t\ty*|Y*) status=0; break ;;\n> -\t\t\tn*|N*) status=1; break ;;\n> +\t\t\ty*|Y*) return 0 ;;\n> +\t\t\tn*|N*) return 1 ;;\n>  \t\t\tesac\n>  \t\tdone\n>  \tfi\n\nNote: The above left in the response not because I have any comment\non or objection to it but because it is relevant to the comment on\nthe next hunk.\n\n> @@ -130,13 +128,10 @@ setup_user_tool () {\n>  \t\tthen\n>  \t\t\ttouch \"$BACKUP\"\n>  \t\t\t( eval $merge_tool_cmd )\n> -\t\t\tstatus=$?\n>  \t\t\tcheck_unchanged\n>  \t\telse\n>  \t\t\t( eval $merge_tool_cmd )\n> -\t\t\tstatus=$?\n>  \t\tfi\n> -\t\treturn $status\n>  \t}\n>  }\n\nThe caller of this funciton used to get the status from running\n$merge_tool_cmd, but now it gets the result from check_unchanged.\n\nMaybe that is more correct thing to report, but this does change the\nbehaviour, no?\n\n    ... goes and looks ...\n\nAhh, the assignment to $status before running check_unchanged was not\ndoing anything useful, because check_unchanged stomped on $status\nbefore it returned anyway.\n\nSo the net effect of this hunk to the caller's is unchanged.  It is\na bit tricky but the end result looks correct.\n"},{"id":"252359","messageId":"xmqqmw7kz73w.fsf@gitster.dls.corp.google.com","threadId":"38021","inReplyTo":"1416532829-68662-1-git-send-email-davvid@gmail.com","subject":"Re: [PATCH 0/3] mergetool/difftool cleanup","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-11-21T19:28:03Z","receivedAt":"2014-11-21T19:28:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"David Aguilar <davvid@gmail.com> writes:\n\n> This is a cleanup series to remove the use of the $status\n> global variable in mergetool/difftool.\n>\n> This should wait until after the current RC series is\n> over but figured I'd send it out.\n>\n> David Aguilar (3):\n>   mergetool--lib: remove use of $status global\n>   difftool--helper: add explicit exit statement\n>   mergetool: simplify conditionals\n>\n>  git-difftool--helper.sh |  2 ++\n>  git-mergetool--lib.sh   | 20 +++++---------------\n>  git-mergetool.sh        | 16 +++++-----------\n>  3 files changed, 12 insertions(+), 26 deletions(-)\n\nLooked quite straight-forward from a cursory read.\n\nI tentatively inserted the attached patch before 1/3.  If the series\nwas done with that extra step as preliminary clean-up, I wouldn't\nhave had to wonder if the hunk at \"@@ -130,13\" was correct.\n\n-- >8 --\nFrom: Junio C Hamano <gitster@pobox.com>\nDate: Fri, 21 Nov 2014 11:17:57 -0800\nSubject: [PATCH] mergetool--lib: remove no-op assignment to $status from setup_user_tool\n\nEven though setup_user_tool assigns the exit status from \"eval\n$merge_tool_cmd\" to $status, the variable is overwritten by the\nfunction it calls next, check_unchanged, without ever getting looked\nat by anybody.  And \"return $status\" at the end of this function\nreturns the value check_unchanged assigned to it (which is the same\nas the value the function returns).  Which makes the assignment a\nno-op.\n\nRemove it.\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n git-mergetool--lib.sh | 1 -\n 1 file changed, 1 deletion(-)\n\ndiff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh\nindex 2b66351..3e06389 100644\n--- a/git-mergetool--lib.sh\n+++ b/git-mergetool--lib.sh\n@@ -130,7 +130,6 @@ setup_user_tool () {\n \t\tthen\n \t\t\ttouch \"$BACKUP\"\n \t\t\t( eval $merge_tool_cmd )\n-\t\t\tstatus=$?\n \t\t\tcheck_unchanged\n \t\telse\n \t\t\t( eval $merge_tool_cmd )\n-- \n2.2.0-rc2-128-ge2b5e8e\n"},{"id":"252374","messageId":"20141122001956.GA70804@gmail.com","threadId":"38021","inReplyTo":"xmqqmw7kz73w.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 0/3] mergetool/difftool cleanup","fromName":"David Aguilar","fromEmail":"davvid@gmail.com","sentAt":"2014-11-22T00:20:00Z","receivedAt":"2014-11-22T00:20:00Z","isPatch":true,"sender":{"key":"davvid@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13196?v=4"},"body":"On Fri, Nov 21, 2014 at 11:28:03AM -0800, Junio C Hamano wrote:\n> David Aguilar <davvid@gmail.com> writes:\n> \n> > This is a cleanup series to remove the use of the $status\n> > global variable in mergetool/difftool.\n> >\n> > This should wait until after the current RC series is\n> > over but figured I'd send it out.\n> >\n> > David Aguilar (3):\n> >   mergetool--lib: remove use of $status global\n> >   difftool--helper: add explicit exit statement\n> >   mergetool: simplify conditionals\n> >\n> >  git-difftool--helper.sh |  2 ++\n> >  git-mergetool--lib.sh   | 20 +++++---------------\n> >  git-mergetool.sh        | 16 +++++-----------\n> >  3 files changed, 12 insertions(+), 26 deletions(-)\n> \n> Looked quite straight-forward from a cursory read.\n> \n> I tentatively inserted the attached patch before 1/3.  If the series\n> was done with that extra step as preliminary clean-up, I wouldn't\n> have had to wonder if the hunk at \"@@ -130,13\" was correct.\n\n\nThat makes a lot of sense, please do insert this patch before\n1/3 (actually 1/4 with the additional patch I sent after the\ninitial submission).\n\nThanks,\nDavid\n\n> \n> -- >8 --\n> From: Junio C Hamano <gitster@pobox.com>\n> Date: Fri, 21 Nov 2014 11:17:57 -0800\n> Subject: [PATCH] mergetool--lib: remove no-op assignment to $status from setup_user_tool\n> \n> Even though setup_user_tool assigns the exit status from \"eval\n> $merge_tool_cmd\" to $status, the variable is overwritten by the\n> function it calls next, check_unchanged, without ever getting looked\n> at by anybody.  And \"return $status\" at the end of this function\n> returns the value check_unchanged assigned to it (which is the same\n> as the value the function returns).  Which makes the assignment a\n> no-op.\n> \n> Remove it.\n> \n> Signed-off-by: Junio C Hamano <gitster@pobox.com>\n> ---\n>  git-mergetool--lib.sh | 1 -\n>  1 file changed, 1 deletion(-)\n> \n> diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh\n> index 2b66351..3e06389 100644\n> --- a/git-mergetool--lib.sh\n> +++ b/git-mergetool--lib.sh\n> @@ -130,7 +130,6 @@ setup_user_tool () {\n>  \t\tthen\n>  \t\t\ttouch \"$BACKUP\"\n>  \t\t\t( eval $merge_tool_cmd )\n> -\t\t\tstatus=$?\n>  \t\t\tcheck_unchanged\n>  \t\telse\n>  \t\t\t( eval $merge_tool_cmd )\n> -- \n> 2.2.0-rc2-128-ge2b5e8e\n> \n"}]}