[TopGit PATCH/RFC] Do not use mktemp

classic Classic list List threaded Threaded
6 messages Options
Reply | Threaded
Open this post in threaded view
|

[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 <[hidden email]>
---
        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

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

[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 <[hidden email]>
---
        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

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

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

Petr Baudis
In reply to this post by Jonathan Nieder
  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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

[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 <[hidden email]>
---
        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: )

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

[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 <[hidden email]>

---
        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: )
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html