[TopGit PATCH/RFC] Do not use mktemp

[TopGit PATCH/RFC] Do not use mktemp

[Jonathan Nieder]

Old operating systems may not even have mktemp; less old
operating systems may have a mktemp which is not compatible with
the usual post-1.5 version.  Let's try to do without.

This patch makes us use /tmp/meaningful-prefix.$$ and
$git_dir/prefix.$$ as temporary filenames when they are
needed.  But there are two exceptions:

 - The needs_update function in tg.sh requires more than one
   temporary file, so we imitate mktemp semantics for it

 - A temporary file is used in tg-patch for some hack I do not
   want to put effort into preserving.  I reused the mktemp
   imitation as a band-aid there.

One word of warning: for needs_update there is a 'trap' to
destroy the temporary directory used, but this overrides any
other traps that the caller might have set before.  Luckily, the
only other use of 'trap' is in tg-export, which never calls
needs_update.  But it leaves me uncomfortable.

Signed-off-by: Jonathan Nieder <jrnieder@uchicago.edu>
---
	On my computer, mktemp requires its template argument;
	on a computer I often ssh into, there is no mktemp at
	all.  So I would rather that topgit didn't use it.  This
	patch has a number of problems, but I thought I would
	send it out anyway to see what people say.

	The biggest issue is that this slows needs_update down,
	since awk 'srand(); rand()' comes up with the same random
	numbers again and again.  This is because the random
	number generator is seeded with the current time *in
	seconds*.  I hope there is some other way...

	Comments welcome.  Thanks,

	Jonathan

 tg-export.sh |    5 +++--
 tg-info.sh   |    4 +++-
 tg-patch.sh  |    3 ++-
 tg-update.sh |    4 +++-
 tg.sh        |   31 ++++++++++++++++++++++++++++++-
 5 files changed, 41 insertions(+), 6 deletions(-)

diff --git a/tg-export.sh b/tg-export.sh
index 73ad2ef..9a232d3 100644
--- a/tg-export.sh
+++ b/tg-export.sh
@@ -31,8 +31,9 @@ name="$(git symbolic-ref HEAD | sed 's#^refs/heads/##')"
 base_rev="$(git rev-parse --short --verify "refs/top-bases/$name" 2>/dev/null)" ||
 	die "not on a TopGit-controlled branch"
 
-
-playground="$(mktemp -d)"
+playground=${TMPDIR:-/tmp}/tg-export-playground.$$
+( umask 077 && mkdir "$playground" ) ||
+	die "cannot make temporary directory: $!"
 trap 'rm -rf "$playground"' EXIT
 
 
diff --git a/tg-info.sh b/tg-info.sh
index 43589f9..c50c9b7 100644
--- a/tg-info.sh
+++ b/tg-info.sh
@@ -41,7 +41,9 @@ branch_contains "$name" "$base_rev" ||
 git cat-file blob "$name:.topdeps" |
 	sed '1{s/^/Depends: /;n}; s/^/         /;'
 
-depcheck="$(mktemp)"
+depcheck=$git_dir/tg-depcheck.$$
+( set -C && umask 077 && : >"$depcheck" ) ||
+	die "cannot make temporary file: $!"
 missing_deps=
 needs_update "$name" >"$depcheck" || :
 if [ -n "$missing_deps" ]; then
diff --git a/tg-patch.sh b/tg-patch.sh
index 04023c0..7517f0f 100644
--- a/tg-patch.sh
+++ b/tg-patch.sh
@@ -29,7 +29,8 @@ echo
 [ -n "$(git grep '^[-]--' "$name" -- ".topmsg")" ] || echo '---'
 
 # Evil obnoxious hack to work around the lack of git diff --exclude
-git_is_stupid="$(mktemp)"
+git_is_stupid=$(temp_filename "$git_dir/tg-patch-tmp.") ||
+	die "$git_is_stupid"
 git diff-tree --name-only "$base_rev" "$name" |
 	fgrep -vx ".topdeps" |
 	fgrep -vx ".topmsg" >"$git_is_stupid" || : # fgrep likes to fail randomly?
diff --git a/tg-update.sh b/tg-update.sh
index 27a8e81..7125f02 100644
--- a/tg-update.sh
+++ b/tg-update.sh
@@ -21,7 +21,9 @@ base_rev="$(git rev-parse --short --verify "refs/top-bases/$name" 2>/dev/null)"
 
 ## First, take care of our base
 
-depcheck="$(mktemp)"
+depcheck=$git_dir/tg-depcheck.$$
+( set -C && umask 077 && : >"$depcheck" ) ||
+	die "cannot make temporary file: $!"
 missing_deps=
 needs_update "$name" >"$depcheck" || :
 [ -z "$missing_deps" ] || die "some dependencies are missing: $missing_deps"
diff --git a/tg.sh b/tg.sh
index e5766fe..c31256f 100644
--- a/tg.sh
+++ b/tg.sh
@@ -80,6 +80,27 @@ branch_contains()
 	[ -z "$(git rev-list ^"$1" "$2")" ]
 }
 
+# temp_filename PREFIX
+# Prints an error message *to standard output* and exits with
+# nonzero status on failure
+temp_filename()
+{
+	set -C && umask 077
+	prefix=$1
+	i=0
+	suffix=$(awk 'BEGIN { srand(); rand(); print int(rand()*99999) }')
+	while test $i -lt 256
+	do
+		tmp=$prefix$suffix
+		: >"$tmp" && break
+		i=$(($i+1))
+		suffix=$(($suffix+1))
+	done
+	test $i -gt 255 &&
+		die "cannot create temporary file: $!"
+	echo $tmp
+}
+
 # recurse_deps CMD NAME [BRANCHPATH...]
 # Recursively eval CMD on all dependencies of NAME.
 # CMD can refer to $_name for queried branch name,
@@ -90,12 +111,15 @@ branch_contains()
 # of the whole function.
 # If recurse_deps() hits missing dependencies, it will append
 # them to space-separated $missing_deps list and skip them.
+# Uses a $recurse_deps_tmp directory, which should be
+# set in advance
 recurse_deps()
 {
 	_cmd="$1"; shift
 	_name="$1"; # no shift
 	_depchain="$*"
-	_depsfile="$(mktemp)"
+	_depsfile=$(temp_filename "$recurse_deps_tmp/") ||
+		die "$_depsfile"
 	git cat-file blob "$_name:.topdeps" >"$_depsfile"
 	_ret=0
 	while read _dep; do
@@ -156,7 +180,12 @@ branch_needs_update()
 # them to space-separated $missing_deps list and skip them.
 needs_update()
 {
+	recurse_deps_tmp=$git_dir/tg-depcheck-tmpdir.$$
+	( umask 077 && mkdir "$recurse_deps_tmp" ) ||
+		die "cannot make temporary directory: $!"
+	trap 'rm -rf "$recurse_deps_tmp"' EXIT
 	recurse_deps branch_needs_update "$@"
+	rm -rf "$recurse_deps_tmp"
 }
 
 # branch_empty NAME
-- 
1.6.0.rc2.531.g79a96

[TopGit PATCH/RFC fixup] suppress "cannot overwrite existing file" error

[Jonathan Nieder]

We had been using sh -C ": >filename" to atomically create a file,
but this has the unfortunate side effect of producing an error
message if the file already exists.  So suppress the error.

Signed-off-by: Jonathan Nieder <jrnieder@uchicago.edu>
---
	Here's a fix to a mistake in the patch I just sent.  If the
	patch was meant for application, I would be suggesting
	squashing this change in.  Sorry for the noise.

 tg.sh |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/tg.sh b/tg.sh
index c31256f..65375d6 100644
--- a/tg.sh
+++ b/tg.sh
@@ -85,14 +85,14 @@ branch_contains()
 # nonzero status on failure
 temp_filename()
 {
-	set -C && umask 077
+	umask 077
 	prefix=$1
 	i=0
 	suffix=$(awk 'BEGIN { srand(); rand(); print int(rand()*99999) }')
 	while test $i -lt 256
 	do
 		tmp=$prefix$suffix
-		: >"$tmp" && break
+		sh -C -c ': >"$tmp"' 2>/dev/null && break
 		i=$(($i+1))
 		suffix=$(($suffix+1))
 	done
-- 
1.6.0.rc2.531.g79a96

Re: [TopGit PATCH/RFC] Do not use mktemp

[Petr Baudis]

  Hi,

On Tue, Aug 12, 2008 at 01:14:27PM -0500, Jonathan Nieder wrote:
> Old operating systems may not even have mktemp; less old
> operating systems may have a mktemp which is not compatible with
> the usual post-1.5 version.  Let's try to do without.

  can you give some concrete examples?

  If there are really systems like that, I have nothing against
providing our own mktemp stub, but I'm not really fond of the approach
chosen in this patch. I see no reason why our stub shouldn't be
compatible with mktemp - what strikes me is a lot of code duplication
and arcane umask use at a lot of places, this should be encapsulated in
some mktemp() function.

-- 
				Petr "Pasky" Baudis
The next generation of interesting software will be done
on the Macintosh, not the IBM PC.  -- Bill Gates

Re: [TopGit PATCH/RFC] Do not use mktemp

[Jonathan Nieder]

Hi,

Petr Baudis wrote:

> On Tue, Aug 12, 2008 at 01:14:27PM -0500, Jonathan Nieder wrote:
> > Old operating systems may not even have mktemp; less old
> > operating systems may have a mktemp which is not compatible with
> > the usual post-1.5 version.  Let's try to do without.
> 
>   can you give some concrete examples?

SunOS 5.8 has no mktemp (which is not so bad, because as you say, we
could provide our own stub).

Mac OS X mktemp does not allow omitting the template argument but
otherwise shouldn't be much of a problem.  On Mac OS X (and old BSDs,
too, presumably), "mktemp -t arg" treats arg as a prefix for a filename
rather than a template for a filename, but that is just a cosmetic
problem.

So thinking it through, perhaps we should do the following:
 - provide a simple mini-mktemp for users install to install themselves
   on old systems
 - always provide the template argument to mktemp

The arcane umask use would then go in that mini-mktemp which most people
would not be using.  I still would appreciate suggestions from any shell
hackers on how to implement it properly (in particular, how to get
random numbers that change more than once/second).

I'll work on a patch in a few hours when I get home.  Thanks for the
comments.

Jonathan

[TopGit PATCH] supply template argument to mktemp

[Jonathan Nieder]

mktemp versions before 1.5 did not allow omitting the template
(prefix.XXXXXX) argument.  Thus on Mac OS X,

	$ mktemp -d
	usage: mktemp [-d] [-q] [-t prefix] [-u] template ...
	       mktemp [-d] [-q] [-u] -t prefix

So supply a filename template.  To maintain the intended
behavior, we have to add -t, too.  It was implied before.

Signed-off-by: Jonathan Nieder <jrnieder@uchicago.edu>
---
	Jonathan Nieder wrote:

	> So thinking it through, perhaps we should do the following:
	>  - provide a simple mini-mktemp for users to install themselves
	>    on old systems
	>  - always provide the template argument to mktemp

	Here's the second part.  With this change, plus a sed
	portability fix I will send separately, topgit on Mac OS X
	works.  Thanks for the help.

 tg-export.sh |    2 +-
 tg-info.sh   |    2 +-
 tg-patch.sh  |    2 +-
 tg-update.sh |    2 +-
 tg.sh        |    2 +-
 5 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/tg-export.sh b/tg-export.sh
index 62361dd..62ea4f9 100644
--- a/tg-export.sh
+++ b/tg-export.sh
@@ -32,7 +32,7 @@ base_rev="$(git rev-parse --short --verify "refs/top-bases/$name" 2>/dev/null)"
 	die "not on a TopGit-controlled branch"
 
 
-playground="$(mktemp -d)"
+playground="$(mktemp -d -t tg-export.XXXXXX)"
 trap 'rm -rf "$playground"' EXIT
 
 
diff --git a/tg-info.sh b/tg-info.sh
index 43589f9..f2b6365 100644
--- a/tg-info.sh
+++ b/tg-info.sh
@@ -41,7 +41,7 @@ branch_contains "$name" "$base_rev" ||
 git cat-file blob "$name:.topdeps" |
 	sed '1{s/^/Depends: /;n}; s/^/         /;'
 
-depcheck="$(mktemp)"
+depcheck="$(mktemp -t tg-depcheck.XXXXXX)"
 missing_deps=
 needs_update "$name" >"$depcheck" || :
 if [ -n "$missing_deps" ]; then
diff --git a/tg-patch.sh b/tg-patch.sh
index 04023c0..48f4d77 100644
--- a/tg-patch.sh
+++ b/tg-patch.sh
@@ -29,7 +29,7 @@ echo
 [ -n "$(git grep '^[-]--' "$name" -- ".topmsg")" ] || echo '---'
 
 # Evil obnoxious hack to work around the lack of git diff --exclude
-git_is_stupid="$(mktemp)"
+git_is_stupid="$(mktemp -t tg-patch-changes.XXXXXX)"
 git diff-tree --name-only "$base_rev" "$name" |
 	fgrep -vx ".topdeps" |
 	fgrep -vx ".topmsg" >"$git_is_stupid" || : # fgrep likes to fail randomly?
diff --git a/tg-update.sh b/tg-update.sh
index 27a8e81..50b29b4 100644
--- a/tg-update.sh
+++ b/tg-update.sh
@@ -21,7 +21,7 @@ base_rev="$(git rev-parse --short --verify "refs/top-bases/$name" 2>/dev/null)"
 
 ## First, take care of our base
 
-depcheck="$(mktemp)"
+depcheck="$(mktemp -t tg-depcheck.XXXXXX)"
 missing_deps=
 needs_update "$name" >"$depcheck" || :
 [ -z "$missing_deps" ] || die "some dependencies are missing: $missing_deps"
diff --git a/tg.sh b/tg.sh
index e5766fe..1bc886a 100644
--- a/tg.sh
+++ b/tg.sh
@@ -95,7 +95,7 @@ recurse_deps()
 	_cmd="$1"; shift
 	_name="$1"; # no shift
 	_depchain="$*"
-	_depsfile="$(mktemp)"
+	_depsfile="$(mktemp -t tg-depsfile.XXXXXX)"
 	git cat-file blob "$_name:.topdeps" >"$_depsfile"
 	_ret=0
 	while read _dep; do
-- 
tg: (f27e693..) t/topgit/mktemp-template (depends on: )

[TopGit PATCH] tg-info: fix sed typo

[Jonathan Nieder]

There was a semicolon missing before a closing brace.  The result
is an error message on some operating systems (e.g., Mac OS 10.3).

	$ tg info
	Topic Branch: t/some-topic (       2/       2 commits)
	Subject: [PATCH] t/some-topic
	Base: 082a7c9
	sed: 1: "1{s/^/Depends: /;n}; s/ ...": extra characters a
	t the end of n command

Signed-off-by: Jonathan Nieder <jrnieder@uchicago.edu>

---
	Hi again,

	Here is the other change I needed to run topgit.  I
	hope it is of some use.

 tg-info.sh |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/tg-info.sh b/tg-info.sh
index 43589f9..7899ada 100644
--- a/tg-info.sh
+++ b/tg-info.sh
@@ -39,7 +39,7 @@ branch_contains "$name" "$base_rev" ||
 	echo "Base is newer than head! Please run \`tg update\`."
 
 git cat-file blob "$name:.topdeps" |
-	sed '1{s/^/Depends: /;n}; s/^/         /;'
+	sed '1{ s/^/Depends: /; n; }; s/^/         /'
 
 depcheck="$(mktemp)"
 missing_deps=
-- 
tg: (f27e693..) t/sed-fix (depends on: )