)]}'
{"/COMMIT_MSG":[{"author":{"_account_id":7118,"name":"Ian Wienand","email":"iwienand@redhat.com","username":"iwienand"},"change_message_id":"991ee0d39b3e1a882a7ead733fc1d192bd13aa7b","unresolved":false,"context_lines":[{"line_number":10,"context_line":"listen to exit 1\u0027s nor do we allow break\u003dafter-error. This is because"},{"line_number":11,"context_line":"the run_d function is called in a subshell in order to grab its output."},{"line_number":12,"context_line":""},{"line_number":13,"context_line":"This also turns on pipefail in the main disk-image-create script."},{"line_number":14,"context_line":""},{"line_number":15,"context_line":"Change-Id: I88ab2e7104148437eabfe6880e3a1e5ebbb2c15d"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":9,"id":"dab17558_fa53866a","line":13,"updated":"2016-05-13 05:09:12.000000000","message":"it\u0027s not doing this any more","commit_id":"5700aaf0f82ffa37cf4ef76505d4d11ed8601014"}],"bin/disk-image-create":[{"author":{"_account_id":7118,"name":"Ian Wienand","email":"iwienand@redhat.com","username":"iwienand"},"change_message_id":"de39945df3be1f9c40e6c76a608ec317863beba1","unresolved":false,"context_lines":[{"line_number":15,"context_line":"# under the License."},{"line_number":16,"context_line":""},{"line_number":17,"context_line":"set -eE"},{"line_number":18,"context_line":"set -o pipefail"},{"line_number":19,"context_line":""},{"line_number":20,"context_line":"# Prevent perl from complaining a lot, but also remove any unexpected side-effects"},{"line_number":21,"context_line":"# of $LANG varying between build hosts"}],"source_content_type":"application/x-shellscript","patch_set":3,"id":"da6ed579_234946d1","line":18,"updated":"2016-01-12 01:34:52.000000000","message":"this is pretty big, might be good to call out in the change description.\n\ni do think it\u0027s better to have it on, however","commit_id":"f64abf6e03cba22c5bba00d8635b94b204c540a5"},{"author":{"_account_id":7118,"name":"Ian Wienand","email":"iwienand@redhat.com","username":"iwienand"},"change_message_id":"f9b04be078c1ade87ac280391830f6b066c1c7a9","unresolved":false,"context_lines":[{"line_number":15,"context_line":"# under the License."},{"line_number":16,"context_line":""},{"line_number":17,"context_line":"set -eE"},{"line_number":18,"context_line":"set -o pipefail"},{"line_number":19,"context_line":""},{"line_number":20,"context_line":"# Prevent perl from complaining a lot, but also remove any unexpected side-effects"},{"line_number":21,"context_line":"# of $LANG varying between build hosts"}],"source_content_type":"application/x-shellscript","patch_set":4,"id":"7a5de9d1_d2a2c798","line":18,"updated":"2016-01-31 23:37:50.000000000","message":"i\u0027m not opposed to this, but i think i\u0027d rather see it in a separate change just so we can isolate out if it causes problems.","commit_id":"e564cc0df868b1d94f0fc1f348605850cf1f95a7"}],"lib/common-functions":[{"author":{"_account_id":7118,"name":"Ian Wienand","email":"iwienand@redhat.com","username":"iwienand"},"change_message_id":"de39945df3be1f9c40e6c76a608ec317863beba1","unresolved":false,"context_lines":[{"line_number":113,"context_line":"# $1 is the hook to run"},{"line_number":114,"context_line":"# $2 is the regex to look for"},{"line_number":115,"context_line":"function eval_run_d () {"},{"line_number":116,"context_line":"    run_output\u003d$(mktemp)"},{"line_number":117,"context_line":"    trap \"rm -f $run_output; check_break after-error ${break_cmd:-bash}\" ERR"},{"line_number":118,"context_line":"    run_d $1 $run_output"},{"line_number":119,"context_line":"    if grep -q \"$2\" $run_output; then"}],"source_content_type":"application/x-shellscript","patch_set":3,"id":"da6ed579_232066f7","line":116,"updated":"2016-01-12 01:34:52.000000000","message":"make this a local?","commit_id":"f64abf6e03cba22c5bba00d8635b94b204c540a5"},{"author":{"_account_id":7118,"name":"Ian Wienand","email":"iwienand@redhat.com","username":"iwienand"},"change_message_id":"de39945df3be1f9c40e6c76a608ec317863beba1","unresolved":false,"context_lines":[{"line_number":117,"context_line":"    trap \"rm -f $run_output; check_break after-error ${break_cmd:-bash}\" ERR"},{"line_number":118,"context_line":"    run_d $1 $run_output"},{"line_number":119,"context_line":"    if grep -q \"$2\" $run_output; then"},{"line_number":120,"context_line":"        TEMP\u003d`grep \"$2\" $run_output`"},{"line_number":121,"context_line":"        eval \"$TEMP\""},{"line_number":122,"context_line":"    fi"},{"line_number":123,"context_line":"    rm $run_output"}],"source_content_type":"application/x-shellscript","patch_set":3,"id":"da6ed579_a38216c7","line":120,"updated":"2016-01-12 01:34:52.000000000","message":"this lost it\u0027s \"local\", and should probably be lower case?","commit_id":"f64abf6e03cba22c5bba00d8635b94b204c540a5"},{"author":{"_account_id":7118,"name":"Ian Wienand","email":"iwienand@redhat.com","username":"iwienand"},"change_message_id":"de39945df3be1f9c40e6c76a608ec317863beba1","unresolved":false,"context_lines":[{"line_number":194,"context_line":"    rm -rf --one-file-system $TMP_IMAGE_DIR"},{"line_number":195,"context_line":"}"},{"line_number":196,"context_line":""},{"line_number":197,"context_line":"# Run a directory of hooks outside the target (that is, no chrooting)."},{"line_number":198,"context_line":"function run_d() {"},{"line_number":199,"context_line":"    check_element"},{"line_number":200,"context_line":"    check_break before-$1 bash"}],"source_content_type":"application/x-shellscript","patch_set":3,"id":"da6ed579_031762a4","line":197,"updated":"2016-01-12 01:34:52.000000000","message":"i think it would be nice to document the usage of this now it has arguments","commit_id":"f64abf6e03cba22c5bba00d8635b94b204c540a5"},{"author":{"_account_id":7118,"name":"Ian Wienand","email":"iwienand@redhat.com","username":"iwienand"},"change_message_id":"f9b04be078c1ade87ac280391830f6b066c1c7a9","unresolved":false,"context_lines":[{"line_number":117,"context_line":"    trap \"rm -f $run_output; check_break after-error ${break_cmd:-bash}\" ERR"},{"line_number":118,"context_line":"    run_d $1 $run_output"},{"line_number":119,"context_line":"    if grep -q \"$2\" $run_output; then"},{"line_number":120,"context_line":"        local temp\u003d`grep \"$2\" $run_output`"},{"line_number":121,"context_line":"        eval \"$temp\""},{"line_number":122,"context_line":"    fi"},{"line_number":123,"context_line":"    rm $run_output"}],"source_content_type":"application/x-shellscript","patch_set":4,"id":"7a5de9d1_52e497df","line":120,"updated":"2016-01-31 23:37:50.000000000","message":"style nit, prefer $( for consistency","commit_id":"e564cc0df868b1d94f0fc1f348605850cf1f95a7"},{"author":{"_account_id":7118,"name":"Ian Wienand","email":"iwienand@redhat.com","username":"iwienand"},"change_message_id":"f9b04be078c1ade87ac280391830f6b066c1c7a9","unresolved":false,"context_lines":[{"line_number":197,"context_line":"# Run a directory of hooks outside the target (that is, no chrooting)."},{"line_number":198,"context_line":"function run_d() {"},{"line_number":199,"context_line":"    check_element"},{"line_number":200,"context_line":"    check_break before-$1 bash"},{"line_number":201,"context_line":"    if [ -d ${TMP_HOOKS_PATH}/$1.d ] ; then"},{"line_number":202,"context_line":"        if [ -n \"$2\" ]; then"},{"line_number":203,"context_line":"            dib-run-parts ${TMP_HOOKS_PATH}/$1.d | tee $2"}],"source_content_type":"application/x-shellscript","patch_set":4,"id":"7a5de9d1_d2c9675a","line":200,"updated":"2016-01-31 23:37:50.000000000","message":"should we fix this up to ${break_cmd:-bash} while here?","commit_id":"e564cc0df868b1d94f0fc1f348605850cf1f95a7"},{"author":{"_account_id":7118,"name":"Ian Wienand","email":"iwienand@redhat.com","username":"iwienand"},"change_message_id":"90a42a64813d2da16d4c41bbaa6201431c9b4fe6","unresolved":false,"context_lines":[{"line_number":200,"context_line":"    check_break before-$1 ${break_cmd:-bash}"},{"line_number":201,"context_line":"    if [ -d ${TMP_HOOKS_PATH}/$1.d ] ; then"},{"line_number":202,"context_line":"        if [ -n \"$2\" ]; then"},{"line_number":203,"context_line":"            dib-run-parts ${TMP_HOOKS_PATH}/$1.d | tee $2"},{"line_number":204,"context_line":"        else"},{"line_number":205,"context_line":"            dib-run-parts ${TMP_HOOKS_PATH}/$1.d"},{"line_number":206,"context_line":"        fi"}],"source_content_type":"application/x-shellscript","patch_set":7,"id":"dae33548_bf78d7f3","line":203,"updated":"2016-02-16 02:49:02.000000000","message":"So I mentioned in previous reviews about setting \"pipefail\" but upon thinking about it, I guess you set that there because otherwise we don\u0027t catch the failure of dib-run-parts here?\n\nSo, if that is the case, then I don\u0027t think this is generically safe anyway because no guarantee someone will call it with pipefail.  we should check PIPESTATUS[0] here and bail correctly, and that will work everywhere","commit_id":"7d054c2b9d6bb04b869136e0e9788209654b7764"},{"author":{"_account_id":10035,"name":"greghaynes","email":"greg@greghaynes.net","username":"greghaynes"},"change_message_id":"5cd4eddee474fad8c869f0d59c4c51b084b80736","unresolved":false,"context_lines":[{"line_number":200,"context_line":"    check_break before-$1 ${break_cmd:-bash}"},{"line_number":201,"context_line":"    if [ -d ${TMP_HOOKS_PATH}/$1.d ] ; then"},{"line_number":202,"context_line":"        if [ -n \"$2\" ]; then"},{"line_number":203,"context_line":"            dib-run-parts ${TMP_HOOKS_PATH}/$1.d | tee $2"},{"line_number":204,"context_line":"        else"},{"line_number":205,"context_line":"            dib-run-parts ${TMP_HOOKS_PATH}/$1.d"},{"line_number":206,"context_line":"        fi"}],"source_content_type":"application/x-shellscript","patch_set":7,"id":"dae33548_367af7f1","line":203,"in_reply_to":"dae33548_bf78d7f3","updated":"2016-02-17 00:43:06.000000000","message":"oh, I think I forgot to submit my reply to that - yes, the pipefail is required for this change to be effective.\n\nI really don\u0027t think we need to consider these bash functions a public API - they aren\u0027t exposed to dib scripts and there really is no hope of calling most of them individually. I do think just setting pipefail here is better, though since I am a bit worried about how large of an effect pipefail could have. I\u0027ll fix that in a followup.","commit_id":"7d054c2b9d6bb04b869136e0e9788209654b7764"},{"author":{"_account_id":7118,"name":"Ian Wienand","email":"iwienand@redhat.com","username":"iwienand"},"change_message_id":"eace76b5fee8d7b5d95ebfe977d9615ca4582965","unresolved":false,"context_lines":[{"line_number":114,"context_line":"# $2 is the regex to look for"},{"line_number":115,"context_line":"function eval_run_d () {"},{"line_number":116,"context_line":"    local run_output\u003d$(mktemp)"},{"line_number":117,"context_line":"    trap \"rm -f $run_output; check_break after-error ${break_cmd:-bash}\" ERR"},{"line_number":118,"context_line":"    run_d $1 $run_output"},{"line_number":119,"context_line":"    if grep -q \"$2\" $run_output; then"},{"line_number":120,"context_line":"        local temp\u003d$(grep \"$2\" $run_output)"}],"source_content_type":"application/x-shellscript","patch_set":8,"id":"3afc51ec_63bfdbd0","line":117,"updated":"2016-03-13 22:55:33.000000000","message":"i\u0027m not sure if this is a -1, but I just grepped for \"break_cmd\" and it\u0027s being used within func_tests for writing out sentinel files.\n\nI think overloading this separate use of breaking with the same variable is probably a bad idea.\n\nI\u0027m guessing you don\u0027t expect users to overwrite this since it\u0027s lower case and not prefixed ... but if we expect to be able to instrument test points, each should be unique.\n\nI think the follow behaves the way you would think\n\n---\n function foobar {\n    break_cmd\u003dbreak_cmd_${FUNCNAME[0]}\n    break_cmd\u003d${!break_cmd:-bash}\n }\n\n break_cmd_foobar\u003d\"hello\"\n foobar\n unset break_cmd_foobar\n foobar\n---\n\nthat way, we can set breaks based on the function easily","commit_id":"24bdaab95273650d237b3ade02974988c5bc5c54"},{"author":{"_account_id":7118,"name":"Ian Wienand","email":"iwienand@redhat.com","username":"iwienand"},"change_message_id":"387e99bbc807ce54d50aaf1a8703bfa974e6bce4","unresolved":false,"context_lines":[{"line_number":201,"context_line":"    if [ -d ${TMP_HOOKS_PATH}/$1.d ] ; then"},{"line_number":202,"context_line":"        if [ -n \"$2\" ]; then"},{"line_number":203,"context_line":"            # Subshell so we can set pipefail and then restore it"},{"line_number":204,"context_line":"            ("},{"line_number":205,"context_line":"                set -o pipefail"},{"line_number":206,"context_line":"                dib-run-parts ${TMP_HOOKS_PATH}/$1.d | tee $2"},{"line_number":207,"context_line":"            )"}],"source_content_type":"application/x-shellscript","patch_set":8,"id":"dae33548_78bf0561","line":204,"updated":"2016-02-18 03:30:05.000000000","message":"I really think it\u0027s better to just check PIPEFAIL here rather than add the complexity of another sub-shell","commit_id":"24bdaab95273650d237b3ade02974988c5bc5c54"},{"author":{"_account_id":7118,"name":"Ian Wienand","email":"iwienand@redhat.com","username":"iwienand"},"change_message_id":"762e45ee6859780d63d3529d2139e4c85e7d73b9","unresolved":false,"context_lines":[{"line_number":201,"context_line":"    if [ -d ${TMP_HOOKS_PATH}/$1.d ] ; then"},{"line_number":202,"context_line":"        if [ -n \"$2\" ]; then"},{"line_number":203,"context_line":"            # Subshell so we can set pipefail and then restore it"},{"line_number":204,"context_line":"            ("},{"line_number":205,"context_line":"                set -o pipefail"},{"line_number":206,"context_line":"                dib-run-parts ${TMP_HOOKS_PATH}/$1.d | tee $2"},{"line_number":207,"context_line":"            )"}],"source_content_type":"application/x-shellscript","patch_set":8,"id":"3afc51ec_6387db61","line":204,"in_reply_to":"3afc51ec_cd9f0002","updated":"2016-03-13 22:48:03.000000000","message":"I find sub-shells rarely work out less complex in the long run.  of course they take slightly longer but that\u0027s minor.  it introduces another process into the hierarchy which confuses things when you\u0027re debugging and introduces points of failure for things to be modified in the subshell that you don\u0027t expect (this has all happened in devstack before :)\n\n dib-run-parts ${TMP_HOOKS_PATH}/$1.d | tee $2\n if [[ $PIPESTATUS[0] !\u003d 0 ]]; then\n   return 1\n fi","commit_id":"24bdaab95273650d237b3ade02974988c5bc5c54"},{"author":{"_account_id":10035,"name":"greghaynes","email":"greg@greghaynes.net","username":"greghaynes"},"change_message_id":"065c7a13dd003493ee530e01aa846c7f6629eb97","unresolved":false,"context_lines":[{"line_number":201,"context_line":"    if [ -d ${TMP_HOOKS_PATH}/$1.d ] ; then"},{"line_number":202,"context_line":"        if [ -n \"$2\" ]; then"},{"line_number":203,"context_line":"            # Subshell so we can set pipefail and then restore it"},{"line_number":204,"context_line":"            ("},{"line_number":205,"context_line":"                set -o pipefail"},{"line_number":206,"context_line":"                dib-run-parts ${TMP_HOOKS_PATH}/$1.d | tee $2"},{"line_number":207,"context_line":"            )"}],"source_content_type":"application/x-shellscript","patch_set":8,"id":"3afc51ec_cd9f0002","line":204,"in_reply_to":"dae33548_78bf0561","updated":"2016-03-13 20:40:17.000000000","message":"This seems more simple to me and less error prone... is there any benefit to doing it the other way?","commit_id":"24bdaab95273650d237b3ade02974988c5bc5c54"},{"author":{"_account_id":21741,"name":"Andreas Florath","email":"andreas@florath.net","username":"florath"},"change_message_id":"4a478c75169b2ab79d9032db3a56a0a6d8bb7fb2","unresolved":false,"context_lines":[{"line_number":109,"context_line":"  [ -d $TMP_HOOKS_PATH ] || generate_hooks"},{"line_number":110,"context_line":"}"},{"line_number":111,"context_line":""},{"line_number":112,"context_line":"# Run a hook, looking for a regex in its stdout, and eval the matched lines."},{"line_number":113,"context_line":"# $1 is the hook to run"},{"line_number":114,"context_line":"# $2 is the regex to look for"},{"line_number":115,"context_line":"function eval_run_d () {"}],"source_content_type":"application/x-shellscript","patch_set":9,"id":"dab17558_87458871","line":112,"updated":"2016-05-09 22:16:08.000000000","message":"Would it be possible to adapt the comment:\nNeither \u0027regexp\u0027 nor \u0027stdout\u0027 is valid any more ;-)","commit_id":"5700aaf0f82ffa37cf4ef76505d4d11ed8601014"},{"author":{"_account_id":7118,"name":"Ian Wienand","email":"iwienand@redhat.com","username":"iwienand"},"change_message_id":"991ee0d39b3e1a882a7ead733fc1d192bd13aa7b","unresolved":false,"context_lines":[{"line_number":109,"context_line":"  [ -d $TMP_HOOKS_PATH ] || generate_hooks"},{"line_number":110,"context_line":"}"},{"line_number":111,"context_line":""},{"line_number":112,"context_line":"# Run a hook, looking for a regex in its stdout, and eval the matched lines."},{"line_number":113,"context_line":"# $1 is the hook to run"},{"line_number":114,"context_line":"# $2 is the regex to look for"},{"line_number":115,"context_line":"function eval_run_d () {"}],"source_content_type":"application/x-shellscript","patch_set":9,"id":"dab17558_1aaaca70","line":112,"in_reply_to":"dab17558_87458871","updated":"2016-05-13 05:09:12.000000000","message":"I think the comment is still correct ... \"$2\" is a regex to grep, and $run_output is the stdout (passed through)","commit_id":"5700aaf0f82ffa37cf4ef76505d4d11ed8601014"},{"author":{"_account_id":7118,"name":"Ian Wienand","email":"iwienand@redhat.com","username":"iwienand"},"change_message_id":"991ee0d39b3e1a882a7ead733fc1d192bd13aa7b","unresolved":false,"context_lines":[{"line_number":204,"context_line":"            ("},{"line_number":205,"context_line":"                set -o pipefail"},{"line_number":206,"context_line":"                dib-run-parts ${TMP_HOOKS_PATH}/$1.d | tee $2"},{"line_number":207,"context_line":"            )"},{"line_number":208,"context_line":"        else"},{"line_number":209,"context_line":"            dib-run-parts ${TMP_HOOKS_PATH}/$1.d"},{"line_number":210,"context_line":"        fi"}],"source_content_type":"application/x-shellscript","patch_set":9,"id":"dab17558_7a94b6e7","line":207,"updated":"2016-05-13 05:09:12.000000000","message":"i\u0027d still prefer to just see a check of PIPESTATUS rather than this in a subshell.  subshells just add another point for odd failures","commit_id":"5700aaf0f82ffa37cf4ef76505d4d11ed8601014"},{"author":{"_account_id":7118,"name":"Ian Wienand","email":"iwienand@redhat.com","username":"iwienand"},"change_message_id":"21240090e64a6bcebd56a4b519b324c5664a5ee2","unresolved":false,"context_lines":[{"line_number":117,"context_line":"    trap \"rm -f $run_output; check_break after-error ${break_cmd:-bash}\" ERR"},{"line_number":118,"context_line":"    run_d $1 $run_output"},{"line_number":119,"context_line":"    if grep -q \"$2\" $run_output; then"},{"line_number":120,"context_line":"        local temp\u003d$(grep \"$2\" $run_output)"},{"line_number":121,"context_line":"        eval \"$temp\""},{"line_number":122,"context_line":"    fi"},{"line_number":123,"context_line":"    rm $run_output"}],"source_content_type":"application/x-shellscript","patch_set":11,"id":"dab17558_8ace1f2b","line":120,"updated":"2016-05-16 01:45:50.000000000","message":"so the only thing here is that \"local \" hides the result of the command, so if this grep fails it won\u0027t hit errexit, but it shouldn\u0027t because we just grepped for it above.  not worth going around again on.","commit_id":"0d1d6bec7c58b2f52e6e1932f58767a02c86068b"}]}