{"thread":{"id":"27357","subject":"[RFC PATCH 1/2] sh-setup: Make die take the error code as param.","startedAt":"2011-05-15T12:42:19Z","lastAt":"2011-05-15T21:26:37Z","messageCount":5,"participants":["Fredrik Gustafsson","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"167888","messageId":"cover.1305462132.git.iveqy@iveqy.com","threadId":"27357","inReplyTo":null,"subject":"[RFC PATCH 0/2] submodule update continue","fromName":"Fredrik Gustafsson","fromEmail":"iveqy@iveqy.com","sentAt":"2011-05-15T12:42:19Z","receivedAt":"2011-05-15T12:42:19Z","isPatch":true,"sender":{"key":"iveqy@iveqy.com","avatar":"https://avatars.githubusercontent.com/u/761743?v=4"},"body":"When running submodule update git dies after the first error. This is\ninconvenient if a submodule is unavailable. Therefore this patch series\nmakes git continue with the next submodule if a submodule is\nunavailable.\n\nThis does only apply to checkout errors, rebase and merge errors will\nresult with a direct exit with code 2.\n\nA checkout error makes git continue to the next submodule and when done\nwith all submodules exit with code 1.\n\nStill missing are three tests (recursive checkout, merge and recurive rebase).\n\nThis is the second iteration of this patch series. The first can be\nfound on:\nhttp://thread.gmane.org/gmane.comp.version-control.git/171895\n\nFredrik Gustafsson (2):\n  sh-setup: Make die take the error code as param.\n  submodule update: continue when a checkout fails\n\n git-sh-setup.sh             |    6 +++++-\n git-submodule.sh            |   42 ++++++++++++++++++++++++++++++++++++------\n t/t7406-submodule-update.sh |   29 +++++++++++++++++++++++++++++\n 3 files changed, 70 insertions(+), 7 deletions(-)\n\n-- \n1.7.5.1.219.ge2152.dirty\n"},{"id":"167886","messageId":"1f45f5634f08cb0f63d61d8b734a2cf7bebf591b.1305462132.git.iveqy@iveqy.com","threadId":"27357","inReplyTo":"cover.1305462132.git.iveqy@iveqy.com","subject":"[RFC PATCH 1/2] sh-setup: Make die take the error code as param.","fromName":"Fredrik Gustafsson","fromEmail":"iveqy@iveqy.com","sentAt":"2011-05-15T12:42:20Z","receivedAt":"2011-05-15T12:42:20Z","isPatch":true,"sender":{"key":"iveqy@iveqy.com","avatar":"https://avatars.githubusercontent.com/u/761743?v=4"},"body":"die used to print all parameters and then exit with code 1. Now die\nprints the first parameter and uses the optional second parameter as the\nexit code. The default exit code is 1.\n\nThis allows scripts to control the exit code when they call die.\n\nAll current git-code only uses the first parameter of die today so this\nchange has no impact on them.\n\nSigned-off-by: Fredrik Gustafsson <iveqy@iveqy.com>\nMentored-by: Jens Lehmann <Jens.Lehmann@web.de>\nMentored-by: Heiko Voigt <hvoigt@hvoigt.net>\n---\n git-sh-setup.sh |    6 +++++-\n 1 files changed, 5 insertions(+), 1 deletions(-)\n\ndiff --git a/git-sh-setup.sh b/git-sh-setup.sh\nindex aa16b83..6aa6c59 100644\n--- a/git-sh-setup.sh\n+++ b/git-sh-setup.sh\n@@ -40,7 +40,11 @@ git_broken_path_fix () {\n # @@BROKEN_PATH_FIX@@\n \n die() {\n-\techo >&2 \"$@\"\n+\techo >&2 \"$1\"\n+\tif test $2\n+\tthen\n+\t\texit $2\n+\tfi\n \texit 1\n }\n \n-- \n1.7.5.1.219.ge2152.dirty\n"},{"id":"167887","messageId":"551b190565139229c6f374eeba11dd8486375a1d.1305462132.git.iveqy@iveqy.com","threadId":"27357","inReplyTo":"cover.1305462132.git.iveqy@iveqy.com","subject":"[RFC PATCH 2/2] submodule update: continue when a checkout fails","fromName":"Fredrik Gustafsson","fromEmail":"iveqy@iveqy.com","sentAt":"2011-05-15T12:42:21Z","receivedAt":"2011-05-15T12:42:21Z","isPatch":true,"sender":{"key":"iveqy@iveqy.com","avatar":"https://avatars.githubusercontent.com/u/761743?v=4"},"body":"When running git submodule update the submodules are checked out in\nalphabetic order. When an update of a submodule fails because of a\ncheckout error, continue to the next submodule and when done with all\nsubmodules, exit with an error.\n\nWe only do this for 'safe' case when a submodule is not marked as\nrebase or merge. When the update of a submodule fails because of a merge\nor rebase, the update will still die immediately to give the user an\nopportunity to resolve any conflicts before continuing.\n\nSince submodule 'b' does not necessarily need to be dependent on\nsubmodule 'a' this behavior is helpful if we have a lot of submodules.\nFor example if some submodules currently experience network problems\nwe can securely continue with the other submodules and the user can\nrevisit the failed one later on.\n\nIt also is helpful if a checkout fails because a submodules working\ndirectory is dirty. Now the user can cleanup the submodule in question\nand another git submodule update will just update the failed submodule\ninstead of all submodules that are ordered alphabetically afterwards.\n\nSigned-off-by: Fredrik Gustafsson <iveqy@iveqy.com>\nMentored-by: Jens Lehmann <Jens.Lehmann@web.de>\nMentored-by: Heiko Voigt <hvoigt@hvoigt.net>\n---\n git-submodule.sh            |   42 ++++++++++++++++++++++++++++++++++++------\n t/t7406-submodule-update.sh |   29 +++++++++++++++++++++++++++++\n 2 files changed, 65 insertions(+), 6 deletions(-)\n\ndiff --git a/git-submodule.sh b/git-submodule.sh\nindex bf110e9..02c41c7 100755\n--- a/git-submodule.sh\n+++ b/git-submodule.sh\n@@ -444,7 +444,8 @@ cmd_update()\n \tfi\n \n \tcloned_modules=\n-\tmodule_list \"$@\" |\n+\tmodule_list \"$@\" | {\n+\terr=\n \twhile read mode sha1 stage path\n \tdo\n \t\tif test \"$stage\" = U\n@@ -525,17 +526,46 @@ cmd_update()\n \t\t\t\t;;\n \t\t\tesac\n \n-\t\t\t(clear_local_git_env; cd \"$path\" && $command \"$sha1\") ||\n-\t\t\tdie \"Unable to $action '$sha1' in submodule path '$path'\"\n-\t\t\tsay \"Submodule path '$path': $msg '$sha1'\"\n+\t\t\tif (clear_local_git_env; cd \"$path\" && $command \"$sha1\")\n+\t\t\tthen\n+\t\t\t\tsay \"Submodule path '$path': $msg '$sha1'\"\n+\t\t\telse\n+\t\t\t\tcase $action in\n+\t\t\t\trebase|merge)\n+\t\t\t\t\tdie \"Unable to $action '$sha1' in submodule path '$path'\" 2\n+\t\t\t\t\t;;\n+\t\t\t\t*)\n+\t\t\t\t\tsay \"Unable to $action '$sha1' in submodule path '$path'\"\n+\t\t\t\t\terr=\"Failed to $action one or more submodule(s)\"\n+\t\t\t\t\tcontinue\n+\t\t\t\t\t;;\n+\t\t\t\tesac\n+\t\t\tfi\n \t\tfi\n \n \t\tif test -n \"$recursive\"\n \t\tthen\n-\t\t\t(clear_local_git_env; cd \"$path\" && eval cmd_update \"$orig_flags\") ||\n-\t\t\tdie \"Failed to recurse into submodule path '$path'\"\n+\t\t\t(clear_local_git_env; cd \"$path\" && eval cmd_update \"$orig_flags\")\n+\t\t\tres=$?\n+\t\t\tif test $res -gt 0\n+\t\t\tthen\n+\t\t\t\tif test $res -eq 1\n+\t\t\t\tthen\n+\t\t\t\t\tsay \"Failed to recurse into submodule path '$path'\"\n+\t\t\t\t\tcontinue\n+\t\t\t\telse\n+\t\t\t\t\tdie \"Failed to recurse into submodule path '$path'\" $res\n+\t\t\t\tfi\n+\t\t\tfi\n \t\tfi\n \tdone\n+\n+\tif test -n \"$err\"\n+\tthen\n+\t\tdie \"$err\"\n+\tfi\n+\n+\t}\n }\n \n set_name_rev () {\ndiff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh\nindex 4f16fcc..e79c4df 100755\n--- a/t/t7406-submodule-update.sh\n+++ b/t/t7406-submodule-update.sh\n@@ -298,4 +298,33 @@ test_expect_success 'submodule update ignores update=rebase config for new submo\n \t)\n '\n \n+test_expect_success 'submodule update continues after checkout error' '\n+\t(cd super &&\n+\t git reset --hard HEAD &&\n+\t git submodule add ../submodule submodule2 &&\n+\t git submodule init &&\n+\t git commit -am \"new_submodule\" &&\n+\t (cd submodule2 &&\n+\t  git rev-parse HEAD > ../expect\n+\t ) &&\n+\t (cd submodule &&\n+\t  test_commit \"update_submodule\" file\n+\t ) &&\n+\t (cd submodule2 &&\n+\t  test_commit \"update_submodule2\" file\n+\t ) &&\n+\t git add submodule &&\n+\t git add submodule2 &&\n+\t git commit -m \"two_new_submodule_commits\" &&\n+\t (cd submodule &&\n+\t \techo \"\" > file\n+\t ) &&\n+\t git checkout HEAD^ &&\n+\t test_must_fail git submodule update &&\n+\t (cd submodule2 &&\n+\t  git rev-parse HEAD > ../actual\n+\t ) &&\n+\t test_cmp expect actual\n+\t) \n+'\n test_done\n-- \n1.7.5.1.219.ge2152.dirty\n"},{"id":"167923","messageId":"7vzkmnofo0.fsf@alter.siamese.dyndns.org","threadId":"27357","inReplyTo":"1f45f5634f08cb0f63d61d8b734a2cf7bebf591b.1305462132.git.iveqy@iveqy.com","subject":"Re: [RFC PATCH 1/2] sh-setup: Make die take the error code as param.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-05-15T20:46:39Z","receivedAt":"2011-05-15T20:46:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Fredrik Gustafsson <iveqy@iveqy.com> writes:\n\n> die used to print all parameters and then exit with code 1. Now die\n> prints the first parameter and uses the optional second parameter as the\n> exit code. The default exit code is 1.\n>\n> This allows scripts to control the exit code when they call die.\n>\n> All current git-code only uses the first parameter of die today so this\n> change has no impact on them.\n\nThere are two issues in that logic.\n\nThe git-sh-setup API is a published interface that third-party Porcelains\ncan use, so it is insufficient to audit only the in-tree code.\n\nBesides, your audit of in-tree code is insufficient. See output from this:\n\n    $ git grep -A1 -n -e 'die .*\\\\$' -- '*.sh'\n\nThe hits in git-bisect.sh and git-rebase--merge.sh both pass two strings.\n\nCan't you instead introduce a new \"die_with_status\" function and use it?\n"},{"id":"167924","messageId":"7viptbodte.fsf@alter.siamese.dyndns.org","threadId":"27357","inReplyTo":"551b190565139229c6f374eeba11dd8486375a1d.1305462132.git.iveqy@iveqy.com","subject":"Re: [RFC PATCH 2/2] submodule update: continue when a checkout fails","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-05-15T21:26:37Z","receivedAt":"2011-05-15T21:26:37Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Fredrik Gustafsson <iveqy@iveqy.com> writes:\n\n> When running git submodule update the submodules are checked out in\n> alphabetic order. When an update of a submodule fails because of a\n> checkout error, continue to the next submodule and when done with all\n> submodules, exit with an error.\n>\n> We only do this for 'safe' case when a submodule is not marked as\n> rebase or merge. When the update of a submodule fails because of a merge\n> or rebase, the update will still die immediately to give the user an\n> opportunity to resolve any conflicts before continuing.\n>\n> Since submodule 'b' does not necessarily need to be dependent on\n> submodule 'a' this behavior is helpful if we have a lot of submodules.\n> For example if some submodules currently experience network problems\n> we can securely continue with the other submodules and the user can\n> revisit the failed one later on.\n>\n> It also is helpful if a checkout fails because a submodules working\n> directory is dirty. Now the user can cleanup the submodule in question\n> and another git submodule update will just update the failed submodule\n> instead of all submodules that are ordered alphabetically afterwards.\n>\n> Signed-off-by: Fredrik Gustafsson <iveqy@iveqy.com>\n> Mentored-by: Jens Lehmann <Jens.Lehmann@web.de>\n> Mentored-by: Heiko Voigt <hvoigt@hvoigt.net>\n\nThanks, even though I find that the logic does not flow as smoothly as it\ncould in the above description, it now has sufficient information. Compared\nto the previous round, it is a vast improvement.\n\nI would rephrase it differently though. The problem description at the\nhighest level should come first.\n\n\t\"git submodule update\" stops at the first error and gives control\n\tback to the user. Only after the user fixes the problematic\n\tsubmodule and runs \"git submodule update\" again, the second error\n\tis found. And the user needs to repeat until all the problems are\n\tfound and fixed one by one. This is tedious.\n\nThen give a high level description of the proposed approach.\n\n\tInstead, the command can remember which submodules it had trouble\n\twith, continue updating the ones it can, and report which ones had\n\terrors at the end. The user can run \"git submodule update\", find\n\tall the ones that need minor fixing (e.g. working tree was dirty)\n\tto fix them in a single pass. Then another \"git submodule update\"\n\tcan be run to update all.\n\nAnd follow up with minor details.\n\n\tNote that the problematic submodules are skipped only when they\n\tare to be integrated with a safer value of submodule.<name>.update\n\toption, namely \"checkout\". Fixing a failure in a submodule that\n\tuses \"rebase\" or \"merge\" may need an involved conflict resolution\n\tby the user, and leaving too many submodules in states that need\n\tresolution would not reduce the mental burden on the user.\n\nWouldn't that be easier to understand? After all \"alphabetical\" is not an\nessential part of the problem, but \"stopping at the first error, with an\narbitrary definition of order of the submodules (which happens to be\nalphabetical)\" is the problem you are addressing, no?\n\nExpressed that way, it may become clearer that it is dubious that stopping\non \"rebase/merge\" is something you should unilaterally enforce; instead,\nthe user may want to be able to configure if the command should skip and\ncontinue in such a case.\n\n>  git-submodule.sh            |   42 ++++++++++++++++++++++++++++++++++++------\n>  t/t7406-submodule-update.sh |   29 +++++++++++++++++++++++++++++\n>  2 files changed, 65 insertions(+), 6 deletions(-)\n>\n> diff --git a/git-submodule.sh b/git-submodule.sh\n> index bf110e9..02c41c7 100755\n> --- a/git-submodule.sh\n> +++ b/git-submodule.sh\n> @@ -525,17 +526,46 @@ cmd_update()\n>  \t\t\t\t;;\n>  \t\t\tesac\n>  \n> -\t\t\t(clear_local_git_env; cd \"$path\" && $command \"$sha1\") ||\n> -\t\t\tdie \"Unable to $action '$sha1' in submodule path '$path'\"\n> -\t\t\tsay \"Submodule path '$path': $msg '$sha1'\"\n> +\t\t\tif (clear_local_git_env; cd \"$path\" && $command \"$sha1\")\n> +\t\t\tthen\n> +\t\t\t\tsay \"Submodule path '$path': $msg '$sha1'\"\n> +\t\t\telse\n> +\t\t\t\tcase $action in\n> +\t\t\t\trebase|merge)\n> +\t\t\t\t\tdie \"Unable to $action '$sha1' in submodule path '$path'\" 2\n\nPer my comments to your 1/2, this would become\n\n\t\t\t\t\tdie_with_status 2 \"Unable to $action '$sha1' in\" \\\n\t\t\t\t        \t\"submodule path '$path'\"\n\nI think.  And 1/2 would result in something like this:\n\n\tdie_with_status () {\n\t\tstatus=$1\n                shift\n                echo >&2 \"$*\"\n                exit $status\n\t}\n\n        die () {\n        \tdie_with_status 1 \"$@\"\n\t}\n\n> +\t\t\t\t\terr=\"Failed to $action one or more submodule(s)\"\n> +\t\t\t\t\tcontinue\n\nSee below...\n\n>  \t\tif test -n \"$recursive\"\n>  \t\tthen\n> +\t\t\t(clear_local_git_env; cd \"$path\" && eval cmd_update \"$orig_flags\")\n> +\t\t\tres=$?\n> +\t\t\tif test $res -gt 0\n> +\t\t\tthen\n> +\t\t\t\tif test $res -eq 1\n> +\t\t\t\tthen\n> +\t\t\t\t\tsay \"Failed to recurse into submodule path '$path'\"\n> +\t\t\t\t\tcontinue\n\nHmm, should this case be err=\"...\"?\n\n> +\t\t\t\telse\n> +\t\t\t\t\tdie \"Failed to recurse into submodule path '$path'\" $res\n> +\t\t\t\tfi\n> +\t\t\tfi\n>  \t\tfi\n>  \tdone\n> +\n> +\tif test -n \"$err\"\n> +\tthen\n> +\t\tdie \"$err\"\n\nI am not sure if you are properly addressing the original problem that you\nare trying to solve.\n\nWhen the first error stops the user, the user needs to fix the situation\nwith that submodule and continue to get another error, and then needs to\ncontinue until all the errors are fixed incrementally, which is tedious.\nBut at least in the current code, when the user gets control back, the\nerror message clearly says which submodule was troublesome, so the user\nknows where to go to fix things to make progress.\n\nBut after your change, the user essentially gets \"Many submodules were\nupdated but there were a few that were skipped. Scroll up and read reams\nof output to find out which ones they were\".  Aren't you converting one\nform of tedium with another?\n\nHow about making $err variable to accumulate the names/paths of submodules\nyou had trouble updating:\n\n\tpunted_submodules= ;# initialization\n\t...\n\n\tif (... $command \"$sha1\")\n        then\n        \tcase \"$action\" in\n                $unsafe_actions)\n                \tdie_with_status 2 \"Unable to $action '$sha1' ...\" ;;\n\t\t*)\n                \tpunted_submodules=\"$punted_submodules$path \"\n                        continue ;;\n\t\tesac\n\nand formulate the error message here to list all of them, perhaps like:\n\n\tif test -n \"$punted_modules\"\n        then\n\t\tdie \"Failed to update some modules: $punted_modules\"\n\tfi\n"}]}