{"thread":{"id":"14965","subject":"[TopGit PATCH/RFC] Do not use mktemp","startedAt":"2008-08-12T18:14:27Z","lastAt":"2008-08-13T05:16:33Z","messageCount":6,"participants":["Jonathan Nieder","Petr Baudis"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"86956","messageId":"Pine.GSO.4.62.0808121309000.18832@harper.uchicago.edu","threadId":"14965","inReplyTo":null,"subject":"[TopGit PATCH/RFC] Do not use mktemp","fromName":"Jonathan Nieder","fromEmail":"jrnieder@uchicago.edu","sentAt":"2008-08-12T18:14:27Z","receivedAt":"2008-08-12T18:14:27Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Old operating systems may not even have mktemp; less old\noperating systems may have a mktemp which is not compatible with\nthe usual post-1.5 version.  Let's try to do without.\n\nThis patch makes us use /tmp/meaningful-prefix.$$ and\n$git_dir/prefix.$$ as temporary filenames when they are\nneeded.  But there are two exceptions:\n\n - The needs_update function in tg.sh requires more than one\n   temporary file, so we imitate mktemp semantics for it\n\n - A temporary file is used in tg-patch for some hack I do not\n   want to put effort into preserving.  I reused the mktemp\n   imitation as a band-aid there.\n\nOne word of warning: for needs_update there is a 'trap' to\ndestroy the temporary directory used, but this overrides any\nother traps that the caller might have set before.  Luckily, the\nonly other use of 'trap' is in tg-export, which never calls\nneeds_update.  But it leaves me uncomfortable.\n\nSigned-off-by: Jonathan Nieder <jrnieder@uchicago.edu>\n---\n\tOn my computer, mktemp requires its template argument;\n\ton a computer I often ssh into, there is no mktemp at\n\tall.  So I would rather that topgit didn't use it.  This\n\tpatch has a number of problems, but I thought I would\n\tsend it out anyway to see what people say.\n\n\tThe biggest issue is that this slows needs_update down,\n\tsince awk 'srand(); rand()' comes up with the same random\n\tnumbers again and again.  This is because the random\n\tnumber generator is seeded with the current time *in\n\tseconds*.  I hope there is some other way...\n\n\tComments welcome.  Thanks,\n\n\tJonathan\n\n tg-export.sh |    5 +++--\n tg-info.sh   |    4 +++-\n tg-patch.sh  |    3 ++-\n tg-update.sh |    4 +++-\n tg.sh        |   31 ++++++++++++++++++++++++++++++-\n 5 files changed, 41 insertions(+), 6 deletions(-)\n\ndiff --git a/tg-export.sh b/tg-export.sh\nindex 73ad2ef..9a232d3 100644\n--- a/tg-export.sh\n+++ b/tg-export.sh\n@@ -31,8 +31,9 @@ name=\"$(git symbolic-ref HEAD | sed 's#^refs/heads/##')\"\n base_rev=\"$(git rev-parse --short --verify \"refs/top-bases/$name\" 2>/dev/null)\" ||\n \tdie \"not on a TopGit-controlled branch\"\n \n-\n-playground=\"$(mktemp -d)\"\n+playground=${TMPDIR:-/tmp}/tg-export-playground.$$\n+( umask 077 && mkdir \"$playground\" ) ||\n+\tdie \"cannot make temporary directory: $!\"\n trap 'rm -rf \"$playground\"' EXIT\n \n \ndiff --git a/tg-info.sh b/tg-info.sh\nindex 43589f9..c50c9b7 100644\n--- a/tg-info.sh\n+++ b/tg-info.sh\n@@ -41,7 +41,9 @@ branch_contains \"$name\" \"$base_rev\" ||\n git cat-file blob \"$name:.topdeps\" |\n \tsed '1{s/^/Depends: /;n}; s/^/         /;'\n \n-depcheck=\"$(mktemp)\"\n+depcheck=$git_dir/tg-depcheck.$$\n+( set -C && umask 077 && : >\"$depcheck\" ) ||\n+\tdie \"cannot make temporary file: $!\"\n missing_deps=\n needs_update \"$name\" >\"$depcheck\" || :\n if [ -n \"$missing_deps\" ]; then\ndiff --git a/tg-patch.sh b/tg-patch.sh\nindex 04023c0..7517f0f 100644\n--- a/tg-patch.sh\n+++ b/tg-patch.sh\n@@ -29,7 +29,8 @@ echo\n [ -n \"$(git grep '^[-]--' \"$name\" -- \".topmsg\")\" ] || echo '---'\n \n # Evil obnoxious hack to work around the lack of git diff --exclude\n-git_is_stupid=\"$(mktemp)\"\n+git_is_stupid=$(temp_filename \"$git_dir/tg-patch-tmp.\") ||\n+\tdie \"$git_is_stupid\"\n git diff-tree --name-only \"$base_rev\" \"$name\" |\n \tfgrep -vx \".topdeps\" |\n \tfgrep -vx \".topmsg\" >\"$git_is_stupid\" || : # fgrep likes to fail randomly?\ndiff --git a/tg-update.sh b/tg-update.sh\nindex 27a8e81..7125f02 100644\n--- a/tg-update.sh\n+++ b/tg-update.sh\n@@ -21,7 +21,9 @@ base_rev=\"$(git rev-parse --short --verify \"refs/top-bases/$name\" 2>/dev/null)\"\n \n ## First, take care of our base\n \n-depcheck=\"$(mktemp)\"\n+depcheck=$git_dir/tg-depcheck.$$\n+( set -C && umask 077 && : >\"$depcheck\" ) ||\n+\tdie \"cannot make temporary file: $!\"\n missing_deps=\n needs_update \"$name\" >\"$depcheck\" || :\n [ -z \"$missing_deps\" ] || die \"some dependencies are missing: $missing_deps\"\ndiff --git a/tg.sh b/tg.sh\nindex e5766fe..c31256f 100644\n--- a/tg.sh\n+++ b/tg.sh\n@@ -80,6 +80,27 @@ branch_contains()\n \t[ -z \"$(git rev-list ^\"$1\" \"$2\")\" ]\n }\n \n+# temp_filename PREFIX\n+# Prints an error message *to standard output* and exits with\n+# nonzero status on failure\n+temp_filename()\n+{\n+\tset -C && umask 077\n+\tprefix=$1\n+\ti=0\n+\tsuffix=$(awk 'BEGIN { srand(); rand(); print int(rand()*99999) }')\n+\twhile test $i -lt 256\n+\tdo\n+\t\ttmp=$prefix$suffix\n+\t\t: >\"$tmp\" && break\n+\t\ti=$(($i+1))\n+\t\tsuffix=$(($suffix+1))\n+\tdone\n+\ttest $i -gt 255 &&\n+\t\tdie \"cannot create temporary file: $!\"\n+\techo $tmp\n+}\n+\n # recurse_deps CMD NAME [BRANCHPATH...]\n # Recursively eval CMD on all dependencies of NAME.\n # CMD can refer to $_name for queried branch name,\n@@ -90,12 +111,15 @@ branch_contains()\n # of the whole function.\n # If recurse_deps() hits missing dependencies, it will append\n # them to space-separated $missing_deps list and skip them.\n+# Uses a $recurse_deps_tmp directory, which should be\n+# set in advance\n recurse_deps()\n {\n \t_cmd=\"$1\"; shift\n \t_name=\"$1\"; # no shift\n \t_depchain=\"$*\"\n-\t_depsfile=\"$(mktemp)\"\n+\t_depsfile=$(temp_filename \"$recurse_deps_tmp/\") ||\n+\t\tdie \"$_depsfile\"\n \tgit cat-file blob \"$_name:.topdeps\" >\"$_depsfile\"\n \t_ret=0\n \twhile read _dep; do\n@@ -156,7 +180,12 @@ branch_needs_update()\n # them to space-separated $missing_deps list and skip them.\n needs_update()\n {\n+\trecurse_deps_tmp=$git_dir/tg-depcheck-tmpdir.$$\n+\t( umask 077 && mkdir \"$recurse_deps_tmp\" ) ||\n+\t\tdie \"cannot make temporary directory: $!\"\n+\ttrap 'rm -rf \"$recurse_deps_tmp\"' EXIT\n \trecurse_deps branch_needs_update \"$@\"\n+\trm -rf \"$recurse_deps_tmp\"\n }\n \n # branch_empty NAME\n-- \n1.6.0.rc2.531.g79a96\n"},{"id":"86958","messageId":"Pine.GSO.4.62.0808121317110.18832@harper.uchicago.edu","threadId":"14965","inReplyTo":"Pine.GSO.4.62.0808121309000.18832@harper.uchicago.edu","subject":"[TopGit PATCH/RFC fixup] suppress \"cannot overwrite existing file\" error","fromName":"Jonathan Nieder","fromEmail":"jrnieder@uchicago.edu","sentAt":"2008-08-12T18:21:13Z","receivedAt":"2008-08-12T18:21:13Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"We had been using sh -C \": >filename\" to atomically create a file,\nbut this has the unfortunate side effect of producing an error\nmessage if the file already exists.  So suppress the error.\n\nSigned-off-by: Jonathan Nieder <jrnieder@uchicago.edu>\n---\n\tHere's a fix to a mistake in the patch I just sent.  If the\n\tpatch was meant for application, I would be suggesting\n\tsquashing this change in.  Sorry for the noise.\n\n tg.sh |    4 ++--\n 1 files changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/tg.sh b/tg.sh\nindex c31256f..65375d6 100644\n--- a/tg.sh\n+++ b/tg.sh\n@@ -85,14 +85,14 @@ branch_contains()\n # nonzero status on failure\n temp_filename()\n {\n-\tset -C && umask 077\n+\tumask 077\n \tprefix=$1\n \ti=0\n \tsuffix=$(awk 'BEGIN { srand(); rand(); print int(rand()*99999) }')\n \twhile test $i -lt 256\n \tdo\n \t\ttmp=$prefix$suffix\n-\t\t: >\"$tmp\" && break\n+\t\tsh -C -c ': >\"$tmp\"' 2>/dev/null && break\n \t\ti=$(($i+1))\n \t\tsuffix=$(($suffix+1))\n \tdone\n-- \n1.6.0.rc2.531.g79a96\n"},{"id":"86970","messageId":"20080812204433.GM10151@machine.or.cz","threadId":"14965","inReplyTo":"Pine.GSO.4.62.0808121309000.18832@harper.uchicago.edu","subject":"Re: [TopGit PATCH/RFC] Do not use mktemp","fromName":"Petr Baudis","fromEmail":"pasky@suse.cz","sentAt":"2008-08-12T20:44:33Z","receivedAt":"2008-08-12T20:44:33Z","isPatch":true,"sender":{"key":"pasky@ucw.cz","avatar":"https://avatars.githubusercontent.com/u/18439?v=4"},"body":"  Hi,\n\nOn Tue, Aug 12, 2008 at 01:14:27PM -0500, Jonathan Nieder wrote:\n> Old operating systems may not even have mktemp; less old\n> operating systems may have a mktemp which is not compatible with\n> the usual post-1.5 version.  Let's try to do without.\n\n  can you give some concrete examples?\n\n  If there are really systems like that, I have nothing against\nproviding our own mktemp stub, but I'm not really fond of the approach\nchosen in this patch. I see no reason why our stub shouldn't be\ncompatible with mktemp - what strikes me is a lot of code duplication\nand arcane umask use at a lot of places, this should be encapsulated in\nsome mktemp() function.\n\n-- \n\t\t\t\tPetr \"Pasky\" Baudis\nThe next generation of interesting software will be done\non the Macintosh, not the IBM PC.  -- Bill Gates\n"},{"id":"86971","messageId":"Pine.GSO.4.62.0808121557290.24244@harper.uchicago.edu","threadId":"14965","inReplyTo":"20080812204433.GM10151@machine.or.cz","subject":"Re: [TopGit PATCH/RFC] Do not use mktemp","fromName":"Jonathan Nieder","fromEmail":"jrnieder@uchicago.edu","sentAt":"2008-08-12T21:07:34Z","receivedAt":"2008-08-12T21:07:34Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nPetr Baudis wrote:\n\n> On Tue, Aug 12, 2008 at 01:14:27PM -0500, Jonathan Nieder wrote:\n> > Old operating systems may not even have mktemp; less old\n> > operating systems may have a mktemp which is not compatible with\n> > the usual post-1.5 version.  Let's try to do without.\n> \n>   can you give some concrete examples?\n\nSunOS 5.8 has no mktemp (which is not so bad, because as you say, we\ncould provide our own stub).\n\nMac OS X mktemp does not allow omitting the template argument but\notherwise shouldn't be much of a problem.  On Mac OS X (and old BSDs,\ntoo, presumably), \"mktemp -t arg\" treats arg as a prefix for a filename\nrather than a template for a filename, but that is just a cosmetic\nproblem.\n\nSo thinking it through, perhaps we should do the following:\n - provide a simple mini-mktemp for users install to install themselves\n   on old systems\n - always provide the template argument to mktemp\n\nThe arcane umask use would then go in that mini-mktemp which most people\nwould not be using.  I still would appreciate suggestions from any shell\nhackers on how to implement it properly (in particular, how to get\nrandom numbers that change more than once/second).\n\nI'll work on a patch in a few hours when I get home.  Thanks for the\ncomments.\n\nJonathan\n"},{"id":"86990","messageId":"Pine.GSO.4.62.0808121911340.1112@harper.uchicago.edu","threadId":"14965","inReplyTo":"Pine.GSO.4.62.0808121557290.24244@harper.uchicago.edu","subject":"[TopGit PATCH] supply template argument to mktemp","fromName":"Jonathan Nieder","fromEmail":"jrnieder@uchicago.edu","sentAt":"2008-08-13T00:17:22Z","receivedAt":"2008-08-13T00:17:22Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"mktemp versions before 1.5 did not allow omitting the template\n(prefix.XXXXXX) argument.  Thus on Mac OS X,\n\n\t$ mktemp -d\n\tusage: mktemp [-d] [-q] [-t prefix] [-u] template ...\n\t       mktemp [-d] [-q] [-u] -t prefix\n\nSo supply a filename template.  To maintain the intended\nbehavior, we have to add -t, too.  It was implied before.\n\nSigned-off-by: Jonathan Nieder <jrnieder@uchicago.edu>\n---\n\tJonathan Nieder wrote:\n\n\t> So thinking it through, perhaps we should do the following:\n\t>  - provide a simple mini-mktemp for users to install themselves\n\t>    on old systems\n\t>  - always provide the template argument to mktemp\n\n\tHere's the second part.  With this change, plus a sed\n\tportability fix I will send separately, topgit on Mac OS X\n\tworks.  Thanks for the help.\n\n tg-export.sh |    2 +-\n tg-info.sh   |    2 +-\n tg-patch.sh  |    2 +-\n tg-update.sh |    2 +-\n tg.sh        |    2 +-\n 5 files changed, 5 insertions(+), 5 deletions(-)\n\ndiff --git a/tg-export.sh b/tg-export.sh\nindex 62361dd..62ea4f9 100644\n--- a/tg-export.sh\n+++ b/tg-export.sh\n@@ -32,7 +32,7 @@ base_rev=\"$(git rev-parse --short --verify \"refs/top-bases/$name\" 2>/dev/null)\"\n \tdie \"not on a TopGit-controlled branch\"\n \n \n-playground=\"$(mktemp -d)\"\n+playground=\"$(mktemp -d -t tg-export.XXXXXX)\"\n trap 'rm -rf \"$playground\"' EXIT\n \n \ndiff --git a/tg-info.sh b/tg-info.sh\nindex 43589f9..f2b6365 100644\n--- a/tg-info.sh\n+++ b/tg-info.sh\n@@ -41,7 +41,7 @@ branch_contains \"$name\" \"$base_rev\" ||\n git cat-file blob \"$name:.topdeps\" |\n \tsed '1{s/^/Depends: /;n}; s/^/         /;'\n \n-depcheck=\"$(mktemp)\"\n+depcheck=\"$(mktemp -t tg-depcheck.XXXXXX)\"\n missing_deps=\n needs_update \"$name\" >\"$depcheck\" || :\n if [ -n \"$missing_deps\" ]; then\ndiff --git a/tg-patch.sh b/tg-patch.sh\nindex 04023c0..48f4d77 100644\n--- a/tg-patch.sh\n+++ b/tg-patch.sh\n@@ -29,7 +29,7 @@ echo\n [ -n \"$(git grep '^[-]--' \"$name\" -- \".topmsg\")\" ] || echo '---'\n \n # Evil obnoxious hack to work around the lack of git diff --exclude\n-git_is_stupid=\"$(mktemp)\"\n+git_is_stupid=\"$(mktemp -t tg-patch-changes.XXXXXX)\"\n git diff-tree --name-only \"$base_rev\" \"$name\" |\n \tfgrep -vx \".topdeps\" |\n \tfgrep -vx \".topmsg\" >\"$git_is_stupid\" || : # fgrep likes to fail randomly?\ndiff --git a/tg-update.sh b/tg-update.sh\nindex 27a8e81..50b29b4 100644\n--- a/tg-update.sh\n+++ b/tg-update.sh\n@@ -21,7 +21,7 @@ base_rev=\"$(git rev-parse --short --verify \"refs/top-bases/$name\" 2>/dev/null)\"\n \n ## First, take care of our base\n \n-depcheck=\"$(mktemp)\"\n+depcheck=\"$(mktemp -t tg-depcheck.XXXXXX)\"\n missing_deps=\n needs_update \"$name\" >\"$depcheck\" || :\n [ -z \"$missing_deps\" ] || die \"some dependencies are missing: $missing_deps\"\ndiff --git a/tg.sh b/tg.sh\nindex e5766fe..1bc886a 100644\n--- a/tg.sh\n+++ b/tg.sh\n@@ -95,7 +95,7 @@ recurse_deps()\n \t_cmd=\"$1\"; shift\n \t_name=\"$1\"; # no shift\n \t_depchain=\"$*\"\n-\t_depsfile=\"$(mktemp)\"\n+\t_depsfile=\"$(mktemp -t tg-depsfile.XXXXXX)\"\n \tgit cat-file blob \"$_name:.topdeps\" >\"$_depsfile\"\n \t_ret=0\n \twhile read _dep; do\n-- \ntg: (f27e693..) t/topgit/mktemp-template (depends on: )\n"},{"id":"87011","messageId":"Pine.GSO.4.62.0808130013480.8904@harper.uchicago.edu","threadId":"14965","inReplyTo":"Pine.GSO.4.62.0808121911340.1112@harper.uchicago.edu","subject":"[TopGit PATCH] tg-info: fix sed typo","fromName":"Jonathan Nieder","fromEmail":"jrnieder@uchicago.edu","sentAt":"2008-08-13T05:16:33Z","receivedAt":"2008-08-13T05:16:33Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"There was a semicolon missing before a closing brace.  The result\nis an error message on some operating systems (e.g., Mac OS 10.3).\n\n\t$ tg info\n\tTopic Branch: t/some-topic (       2/       2 commits)\n\tSubject: [PATCH] t/some-topic\n\tBase: 082a7c9\n\tsed: 1: \"1{s/^/Depends: /;n}; s/ ...\": extra characters a\n\tt the end of n command\n\nSigned-off-by: Jonathan Nieder <jrnieder@uchicago.edu>\n\n---\n\tHi again,\n\n\tHere is the other change I needed to run topgit.  I\n\thope it is of some use.\n\n tg-info.sh |    2 +-\n 1 files changed, 1 insertions(+), 1 deletions(-)\n\ndiff --git a/tg-info.sh b/tg-info.sh\nindex 43589f9..7899ada 100644\n--- a/tg-info.sh\n+++ b/tg-info.sh\n@@ -39,7 +39,7 @@ branch_contains \"$name\" \"$base_rev\" ||\n \techo \"Base is newer than head! Please run \\`tg update\\`.\"\n \n git cat-file blob \"$name:.topdeps\" |\n-\tsed '1{s/^/Depends: /;n}; s/^/         /;'\n+\tsed '1{ s/^/Depends: /; n; }; s/^/         /'\n \n depcheck=\"$(mktemp)\"\n missing_deps=\n-- \ntg: (f27e693..) t/sed-fix (depends on: )\n"}]}