{"thread":{"id":"32269","subject":"[PATCH v2] t9402: sed -i is not portable","startedAt":"2012-12-04T19:44:48Z","lastAt":"2012-12-04T20:52:00Z","messageCount":2,"participants":["Torsten Bögershausen","Junio C Hamano"],"isPatch":true,"patchVersion":2,"patchTotal":null},"messages":[{"id":"204499","messageId":"201212042044.49477.tboegi@web.de","threadId":"32269","inReplyTo":null,"subject":"[PATCH v2] t9402: sed -i is not portable","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2012-12-04T19:44:48Z","receivedAt":"2012-12-04T19:44:48Z","isPatch":true,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"On some systems sed allows the usage of e.g.\nsed -i -e \"s/line1/line2/\" afile\nto edit the file \"in place\".\nOther systems don't allow that: one observed behaviour is that\nsed -i -e \"s/line1/line2/\" afile\ncreates a backup file called afile-e, which breaks the test.\nAs sed -i is not part of POSIX, avoid it.\n\nUse test_cmp, makes the test easier to debug.\nChain all shell commands with && to detect all kinds of failure.\n\nSigned-off-by: Torsten Bögershausen <tboegi@web.de>\n---\nChanges since v1:\nNo correction of TABs to make it easier to review\n\nIf this is OK:\nMatthew would you like to send a complete re-roll,\nbecause the credit should be on you ?\n\n t/t9402-git-cvsserver-refs.sh | 44 ++++++++++++++++++++++---------------------\n 1 file changed, 23 insertions(+), 21 deletions(-)\n\ndiff --git a/t/t9402-git-cvsserver-refs.sh b/t/t9402-git-cvsserver-refs.sh\nindex 858ef0f..5138f14 100755\n--- a/t/t9402-git-cvsserver-refs.sh\n+++ b/t/t9402-git-cvsserver-refs.sh\n@@ -28,27 +28,26 @@ check_file() {\n }\n \n check_end_tree() {\n-    sandbox=\"$1\"\n-    expectCount=$(wc -l < \"$WORKDIR/check.list\")\n-    cvsCount=$(find \"$sandbox\" -name CVS -prune -o -type f -print | wc -l)\n-    test x\"$cvsCount\" = x\"$expectCount\"\n-    stat=$?\n-    echo \"check_end $sandbox : $stat cvs=$cvsCount expect=$expectCount\" \\\n-\t>> \"$WORKDIR/check.log\"\n-    return $stat\n+    sandbox=\"$1\" &&\n+    wc -l < \"$WORKDIR/check.list\" > expected &&\n+    find \"$sandbox\" -type f | grep -v \"/CVS\" > \"$WORKDIR/check.cvsCount\" &&\n+    wc -l < \"$WORKDIR/check.cvsCount\" >actual &&\n+    test_cmp expected actual &&\n+\t\trm expected actual &&\n+\t\tsort < \"$WORKDIR/check.list\" > expected &&\n+\t\tsort < \"$WORKDIR/check.cvsCount\" | sed -e \"s%cvswork/%%\" >actual &&\n+    test_cmp expected actual &&\n+\t\trm expected actual\n }\n \n check_end_full_tree() {\n-    sandbox=\"$1\"\n-    ver=\"$2\"\n-    expectCount=$(wc -l < \"$WORKDIR/check.list\")\n-    cvsCount=$(find \"$sandbox\" -name CVS -prune -o -type f -print | wc -l)\n-    gitCount=$(git ls-tree -r \"$2\" | wc -l)\n-    test x\"$cvsCount\" = x\"$expectCount\" -a x\"$gitCount\" = x\"$expectCount\"\n-    stat=$?\n-    echo \"check_end $sandbox : $stat cvs=$cvsCount git=$gitCount expect=$expectCount\" \\\n-\t>> \"$WORKDIR/check.log\"\n-    return $stat\n+    sandbox=\"$1\" &&\n+    sort < \"$WORKDIR/check.list\" >expected &&\n+    find \"$sandbox\" -name CVS -prune -o -type f -print | sed -e \"s%$sandbox/%%\" | sort >act1 &&\n+\t\ttest_cmp expected act1 &&\n+    git ls-tree -r \"$2\" | sed -e \"s/^.*blob [0-9a-fA-F]*[\t ]*//\" | sort > act2 &&\n+\t\ttest_cmp expected act2 &&\n+    rm expected act1 act2\n }\n \n #########\n@@ -155,7 +154,8 @@ test_expect_success 'cvs co b1 [cvswork3]' '\n \n test_expect_success 'edit cvswork3 and save diff' '\n     ( cd cvswork3 &&\n-      sed -i -e \"s/line1/line1 - data/\" adir/afile &&\n+      sed -e \"s/line1/line1 - data/\" adir/afile >adir/afileNEW &&\n+\t\t\tmv -f adir/afileNEW adir/afile &&\n       echo \"afile5\" > adir/afile5 &&\n       rm t2 &&\n       cvs -f add adir/afile5 &&\n@@ -168,7 +168,8 @@ test_expect_success 'setup v1.2 on b1' '\n     git checkout b1 &&\n     echo \"new v1.2\" > t3 &&\n     rm t2 &&\n-    sed -i -e \"s/line3/line3 - more data/\" adir/afile &&\n+    sed -e \"s/line3/line3 - more data/\" adir/afile >adir/afileNEW &&\n+\t\tmv -f adir/afileNEW adir/afile &&\n     rm adir/a2file &&\n     echo \"a3file\" >> adir/a3file &&\n     echo \"bfile line 3\" >> adir/bdir/bfile &&\n@@ -300,7 +301,8 @@ test_expect_success 'root dir rm file [cvswork2]' '\n \n test_expect_success 'subdir edit/add/rm files [cvswork2' '\n     ( cd cvswork2 &&\n-      sed -i -e \"s/line 1/line 1 (v2)/\" adir/bdir/bfile &&\n+      sed -e \"s/line 1/line 1 (v2)/\" adir/bdir/bfile >adir/bdir/bfileNEW &&\n+      mv -f adir/bdir/bfileNEW adir/bdir/bfile &&\n       rm adir/bdir/b2file &&\n       cd adir &&\n       cvs -f rm bdir/b2file &&\n-- \n1.8.0.197.g5a90748\n"},{"id":"204502","messageId":"7vfw3lmtfz.fsf@alter.siamese.dyndns.org","threadId":"32269","inReplyTo":"201212042044.49477.tboegi@web.de","subject":"Re: [PATCH v2] t9402: sed -i is not portable","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-12-04T20:52:00Z","receivedAt":"2012-12-04T20:52:00Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Torsten Bögershausen <tboegi@web.de> writes:\n\n> On some systems sed allows the usage of e.g.\n> sed -i -e \"s/line1/line2/\" afile\n> to edit the file \"in place\".\n> Other systems don't allow that: one observed behaviour is that\n> sed -i -e \"s/line1/line2/\" afile\n> creates a backup file called afile-e, which breaks the test.\n> As sed -i is not part of POSIX, avoid it.\n\nThanks.\n\nThe intention is good, but see comments on this part in the patch\nbelow.\n\n> Use test_cmp, makes the test easier to debug.\n\nI see a few other remaining issues in the code after this patch is\napplied.  If we are doing other fixes like these, we may want to fix\nthem as well:\n\n - test_must_fail is used to run cvs; it shouldn't (instead of\n   saying \"test_must_fail cvs ...\", just say \"! cvs ...\").\n\n - A shell function should begin like this: \"shellfunc () {\"\n\n - Redirection should not have SP before the filename (i.e. \">out\",\n   not \"> out\").\n\n - It is OK to spell single-liner subshell like this:\n\n    ( cd cvswork3 && ! cvs -f diff -N -u ) &&\n    ...\n\n   but a multi-line subshell should start its first command on a\n   separate line, like this:\n\n    (\n        cd cvswork3 &&\n        cvs update foo\n    ) &&\n    ...\n\n - quite a few \"[cvswork$n]\" comments are truncated to \"[cvswork$n\"\n   in the test titles.\n\n> Chain all shell commands with && to detect all kinds of failure.\n\nI think you missed the one in 'merge early [cvswork3] b3 with b1'\nwhere a merge is done in a subshell; a failed merge is not detected.\n\nThis is another reason why I asked you not to mix different things\nin a single patch; if this patch is rerolled to cover missing \"&&\"\nthat you missed, it needs to be re-reviewed for the \"sed -i\" fix we\nreview during this round again.\n\n> Signed-off-by: Torsten Bögershausen <tboegi@web.de>\n> ---\n> Changes since v1:\n> No correction of TABs to make it easier to review\n>\n> If this is OK:\n> Matthew would you like to send a complete re-roll,\n> because the credit should be on you ?\n>\n>  t/t9402-git-cvsserver-refs.sh | 44 ++++++++++++++++++++++---------------------\n>  1 file changed, 23 insertions(+), 21 deletions(-)\n>\n> diff --git a/t/t9402-git-cvsserver-refs.sh b/t/t9402-git-cvsserver-refs.sh\n> index 858ef0f..5138f14 100755\n> --- a/t/t9402-git-cvsserver-refs.sh\n> +++ b/t/t9402-git-cvsserver-refs.sh\n> @@ -28,27 +28,26 @@ check_file() {\n>  }\n>  \n>  check_end_tree() {\n> -    sandbox=\"$1\"\n> -    expectCount=$(wc -l < \"$WORKDIR/check.list\")\n> -    cvsCount=$(find \"$sandbox\" -name CVS -prune -o -type f -print | wc -l)\n> -    test x\"$cvsCount\" = x\"$expectCount\"\n> -    stat=$?\n> -    echo \"check_end $sandbox : $stat cvs=$cvsCount expect=$expectCount\" \\\n> -\t>> \"$WORKDIR/check.log\"\n> -    return $stat\n> +    sandbox=\"$1\" &&\n> +    wc -l < \"$WORKDIR/check.list\" > expected &&\n> +    find \"$sandbox\" -type f | grep -v \"/CVS\" > \"$WORKDIR/check.cvsCount\" &&\n> +    wc -l < \"$WORKDIR/check.cvsCount\" >actual &&\n\nYou replaced the original \"find\" that was perfectly correct with\n\"pipe to grep -v\" which is more expensive.  Why?\n\nThe file stores the lines to be counted; why is it called a \"Count\"?\n\nAs \"check.list\" is really the \"expected list of files\", it may want\nto be renamed to \"list.expected\" or something like that.  Then the\noutput from this \"find\" would naturally become \"list.actual\", i.e.\n\n\tfind \"$sandbox\" -name CVS -type d -prune \\\n            -o -type f -print >\"$WORKDIR/list.actual\"\n\nEven though you are comparing only the number of lines in the file\nin this function, are they expected to name the same set of paths?\nThe other function check_end_full_tree used to compare only the\ncount, but you changed it to compare the actul names of paths, which\nmay be a more robust and correct test, so it might make more sense\nto redefine $WORKDIR/list.{expect,actual} to be a sorted list of\npaths relative to the top of the working tree and do something like\nthis:\n\n\t(\n        \tcd \"$sandbox\" &&\n\t        find . -name CVS -type d -prune -o -type f -print\n\t) | sed -e \"s|^./||\" | sort >\"$WORKDIR/list.actual\"\n\nand make sure \"$WORKDIR/list.expect\" is also sorted.  Then you can\nlose the \"wc -l\" based check from here and compare the list of\npaths.\n\n> +    test_cmp expected actual &&\n> +\t\trm expected actual &&\n> +\t\tsort < \"$WORKDIR/check.list\" > expected &&\n> +\t\tsort < \"$WORKDIR/check.cvsCount\" | sed -e \"s%cvswork/%%\" >actual &&\n\nIt is probably easier for a later patch to fix the indentation, if\nthese new lines followed the existing indentation.\n\n> +    test_cmp expected actual &&\n> +\t\trm expected actual\n>  }\n>  \n>  check_end_full_tree() {\n> -    sandbox=\"$1\"\n> -    ver=\"$2\"\n> -    expectCount=$(wc -l < \"$WORKDIR/check.list\")\n> -    cvsCount=$(find \"$sandbox\" -name CVS -prune -o -type f -print | wc -l)\n> -    gitCount=$(git ls-tree -r \"$2\" | wc -l)\n> -    test x\"$cvsCount\" = x\"$expectCount\" -a x\"$gitCount\" = x\"$expectCount\"\n> -    stat=$?\n> -    echo \"check_end $sandbox : $stat cvs=$cvsCount git=$gitCount expect=$expectCount\" \\\n> -\t>> \"$WORKDIR/check.log\"\n> -    return $stat\n> +    sandbox=\"$1\" &&\n> +    sort < \"$WORKDIR/check.list\" >expected &&\n> +    find \"$sandbox\" -name CVS -prune -o -type f -print | sed -e \"s%$sandbox/%%\" | sort >act1 &&\n> +\t\ttest_cmp expected act1 &&\n> +    git ls-tree -r \"$2\" | sed -e \"s/^.*blob [0-9a-fA-F]*[\t ]*//\" | sort > act2 &&\n\nYou can say \"git ls-tree --name-only -r\" and lose the sed.\n"}]}