threads / discuss / 25406

[TopGit PATCHv2] provide global temporary directory

Subject: [TopGit PATCHv2] provide global temporary directory

## tl;dr

3 messages between Oct 9, 2010 and Oct 10, 2010.

replies: 2people: 2as markdown or json

Bert Wesarg· Oct 9, 2010, 21:41 UTC · lore

The standard procedure 'tmp=mktemp; trap "rm $tmp" 0' was broken with the introduction of the pager. Which overwrites the trap itself to close and remove the pager fifo.

Now tg provides a temp playground and all other temp files should be created inside this directory and only this directory will be removed with the exit trap. setup_pager still overwrites the trap, but keeps the rm command from the global temp directory. To simplify this the new function get_temp() is provided.

Signed-off-by: Bert Wesarg <bert.wesarg@googlemail.com>
---
 hooks/pre-commit.sh |    3 +--
 tg-export.sh        |    3 +--
 tg-info.sh          |    3 +--
 tg-mail.sh          |    4 +---
 tg-patch.sh         |    3 +--
 tg-push.sh          |    3 +--
 tg-summary.sh       |    3 +--
 tg-update.sh        |    3 +--
 tg.sh               |   27 +++++++++++++++++++--------
 9 files changed, 27 insertions(+), 25 deletions(-)
diff --git a/hooks/pre-commit.sh b/hooks/pre-commit.sh
index 4f2f16f..848a929 100644 hooks/pre-commit.sh
--- a/hooks/pre-commit.sh
+++ b/hooks/pre-commit.sh
@@ -95,9 +95,8 @@ BEGIN      { in_hunk = 0; }
 	done
 
 # check for repetitions of deps
-depdir="$(mktemp -t -d tg-depdir.XXXXXX)" ||
+depdir="$(get_temp -d tg-depdir)" ||
 	die "Can't check for multiple occurrences of deps"
-trap "rm -rf '$depdir'" 0
 cat_file "(i):.topdeps" |
 	while read dep; do
 		[ ! -d "$depdir/$dep" ] ||
diff --git a/tg-export.sh b/tg-export.sh
index 6d82d55..921e933 100644 tg-export.sh
--- a/tg-export.sh
+++ b/tg-export.sh
@@ -57,8 +57,7 @@ if [ -z "$branches" ]; then
 fi
 
 
-playground="$(mktemp -d -t tg-export.XXXXXX)"
-trap 'rm -rf "$playground"' EXIT
+playground="$(get_temp tg-export -d)"
 
 
 ## Collapse driver
diff --git a/tg-info.sh b/tg-info.sh
index 7d6a34c..10e257e 100644 tg-info.sh
--- a/tg-info.sh
+++ b/tg-info.sh
@@ -51,7 +51,7 @@ fi
 git cat-file blob "$name:.topdeps" |
 	sed '1{ s/^/Depends: /; n; }; s/^/         /;'
 
-depcheck="$(mktemp -t tg-depcheck.XXXXXX)"
+depcheck="$(get_temp tg-depcheck)"
 missing_deps=
 needs_update "$name" >"$depcheck" || :
 if [ -n "$missing_deps" ]; then
@@ -72,6 +72,5 @@ if [ -s "$depcheck" ]; then
 else
 	echo "Up-to-date."
 fi
-rm "$depcheck"
 
 # vim:noet
diff --git a/tg-mail.sh b/tg-mail.sh
index 8167ade..dd4a95a 100644 tg-mail.sh
--- a/tg-mail.sh
+++ b/tg-mail.sh
@@ -34,7 +34,7 @@ if [ -n "$in_reply_to" ]; then
 fi
 
 
-patchfile="$(mktemp -t tg-mail.XXXXXX)"
+patchfile="$(get_temp tg-mail)"
 
 $tg patch "$name" >"$patchfile"
 
@@ -54,6 +54,4 @@ people=
 # NOTE: git-send-email handles cc itself
 eval git send-email $send_email_args "$people" "$patchfile"
 
-rm "$patchfile"
-
 # vim:noet
diff --git a/tg-patch.sh b/tg-patch.sh
index 7bafdfe..68efcf0 100644 tg-patch.sh
--- a/tg-patch.sh
+++ b/tg-patch.sh
@@ -51,7 +51,7 @@ echo
 [ -n "$(git grep $diff_opts '^[-]--' ${diff_committed_only:+"$name"} -- ".topmsg")" ] || echo '---'
 
 # Evil obnoxious hack to work around the lack of git diff --exclude
-git_is_stupid="$(mktemp -t tg-patch-changes.XXXXXX)"
+git_is_stupid="$(get_temp tg-patch-changes)"
 git diff --name-only $diff_opts "$base_rev" ${diff_committed_only:+"$name"} -- |
 	fgrep -vx ".topdeps" |
 	fgrep -vx ".topmsg" >"$git_is_stupid" || : # fgrep likes to fail randomly?
@@ -61,7 +61,6 @@ if [ -s "$git_is_stupid" ]; then
 else
 	echo "No changes."
 fi
-rm "$git_is_stupid"
 
 echo '-- '
 echo "tg: ($base_rev..) $name (depends on: $(cat_file "$topic:.topdeps" | paste -s -d' '))"
diff --git a/tg-push.sh b/tg-push.sh
index 199d738..a928fba 100644 tg-push.sh
--- a/tg-push.sh
+++ b/tg-push.sh
@@ -45,8 +45,7 @@ for name in $branches; do
 	ref_exists "$name" || die "detached HEAD? Can't push $name"
 done
 
-_listfile="$(mktemp -t tg-push-listfile.XXXXXX)"
-trap "rm -f \"$_listfile\"" 0
+_listfile="$(get_temp tg-push-listfile)"
 
 push_branch()
 {
diff --git a/tg-summary.sh b/tg-summary.sh
index af16888..612bd27 100644 tg-summary.sh
--- a/tg-summary.sh
+++ b/tg-summary.sh
@@ -55,10 +55,9 @@ EOT
 fi
 
 if [ -n "$sort" ]; then
-	tsort_input=`mktemp`
+	tsort_input="$(get_temp tg-summary-sort)"
 	exec 4>$tsort_input
 	exec 5<$tsort_input
-	rm $tsort_input
 fi
 
 ## List branches
diff --git a/tg-update.sh b/tg-update.sh
index b256c7c..5162c52 100644 tg-update.sh
--- a/tg-update.sh
+++ b/tg-update.sh
@@ -27,7 +27,7 @@ base_rev="$(git rev-parse --short --verify "refs/top-bases/$name" 2>/dev/null)"
 
 ## First, take care of our base
 
-depcheck="$(mktemp -t tg-depcheck.XXXXXX)"
+depcheck="$(get_temp tg-depcheck)"
 missing_deps=
 needs_update "$name" >"$depcheck" || :
 [ -z "$missing_deps" ] || die "some dependencies are missing: $missing_deps"
@@ -96,7 +96,6 @@ if [ -s "$depcheck" ]; then
 else
 	info "The base is up-to-date."
 fi
-rm "$depcheck"
 
 # Home, sweet home...
 # (We want to always switch back, in case we were on the base from failed
diff --git a/tg.sh b/tg.sh
index 8264a3b..3805eeb 100644 tg.sh
--- a/tg.sh
+++ b/tg.sh
@@ -150,7 +150,7 @@ recurse_deps()
 	_name="$1"; # no shift
 	_depchain="$*"
 
-	_depsfile="$(mktemp -t tg-depsfile.XXXXXX)"
+	_depsfile="$(get_temp tg-depsfile)"
 	# If no_remotes is unset check also our base against remote base.
 	# Checking our head against remote head has to be done in the helper.
 	if test -z "$no_remotes" && has_remote "top-bases/$_name"; then
@@ -183,7 +183,6 @@ recurse_deps()
 		eval "$_cmd"
 	done <"$_depsfile"
 	missing_deps="${missing_deps# }"
-	rm "$_depsfile"
 	return $_ret
 }
 
@@ -334,19 +333,28 @@ setup_pager()
 	# now spawn pager
 	export LESS="${LESS:-FRSX}"	# as in pager.c:pager_preexec()
 
-	_pager_fifo_dir="$(mktemp -t -d tg-pager-fifo.XXXXXX)"
-	_pager_fifo="$_pager_fifo_dir/0"
-	mkfifo -m 600 "$_pager_fifo"
+	# setup_pager should be called only once per command
+	pager_fifo="$tg_tmp_dir/pager"
+	mkfifo -m 600 "$pager_fifo"
 
-	"$TG_PAGER" < "$_pager_fifo" &
-	exec > "$_pager_fifo"		# dup2(pager_fifo.in, 1)
+	"$TG_PAGER" < "$pager_fifo" &
+	exec > "$pager_fifo"		# dup2(pager_fifo.in, 1)
 
 	# this is needed so e.g. `git diff` will still colorize it's output if
 	# requested in ~/.gitconfig with color.diff=auto
 	export GIT_PAGER_IN_USE=1
 
 	# atexit(close(1); wait pager)
-	trap "exec >&-; rm \"$_pager_fifo\"; rmdir \"$_pager_fifo_dir\"; wait" EXIT
+	# deliberately overwrites the global EXIT trap
+	trap "exec >&-; rm -rf \"$tg_tmp_dir\"; wait" EXIT
+}
+
+# get_temp NAME [-d]
+# creates a new temporary file (or directory with -d) in the global
+# temporary directory $tg_tmp_dir with pattern prefix NAME
+get_temp()
+{
+	mktemp ${2-} "$tg_tmp_dir/$1.XXXXXX"
 }
 
 ## Startup
@@ -366,6 +374,9 @@ tg="tg"
 # make sure merging the .top* files will always behave sanely
 setup_ours
 setup_hook "pre-commit"
+# create global temporary directories, inside GIT_DIR
+tg_tmp_dir="$(mktemp -d "$git_dir/tg-tmp.XXXXXX")"
+trap "rm -rf \"$tg_tmp_dir\"" EXIT
 
 ## Dispatch
 
-- 
1.7.1.1067.g5aeb7
Uwe Kleine-König· Oct 9, 2010, 21:53 UTC · re: Bert Wesarg · lore

Re: [TopGit PATCHv2] provide global temporary directory

On Sat, Oct 09, 2010 at 11:41:47PM +0200, Bert Wesarg wrote:
Show 212 quoted lines
> The standard procedure 'tmp=mktemp; trap "rm $tmp" 0' was broken with the
> introduction of the pager. Which overwrites the trap itself to close and
> remove the pager fifo.
> 
> Now tg provides a temp playground and all other temp files should be created
> inside this directory and only this directory will be removed with the exit
> trap. setup_pager still overwrites the trap, but keeps the rm command from
> the global temp directory. To simplify this the new function get_temp() is
> provided.
> 
> Signed-off-by: Bert Wesarg <bert.wesarg@googlemail.com>
> ---
>  hooks/pre-commit.sh |    3 +--
>  tg-export.sh        |    3 +--
>  tg-info.sh          |    3 +--
>  tg-mail.sh          |    4 +---
>  tg-patch.sh         |    3 +--
>  tg-push.sh          |    3 +--
>  tg-summary.sh       |    3 +--
>  tg-update.sh        |    3 +--
>  tg.sh               |   27 +++++++++++++++++++--------
>  9 files changed, 27 insertions(+), 25 deletions(-)
> 
> diff --git a/hooks/pre-commit.sh b/hooks/pre-commit.sh
> index 4f2f16f..848a929 100644 hooks/pre-commit.sh
> --- a/hooks/pre-commit.sh
> +++ b/hooks/pre-commit.sh
> @@ -95,9 +95,8 @@ BEGIN      { in_hunk = 0; }
>  	done
>  
>  # check for repetitions of deps
> -depdir="$(mktemp -t -d tg-depdir.XXXXXX)" ||
> +depdir="$(get_temp -d tg-depdir)" ||
>  	die "Can't check for multiple occurrences of deps"
> -trap "rm -rf '$depdir'" 0
>  cat_file "(i):.topdeps" |
>  	while read dep; do
>  		[ ! -d "$depdir/$dep" ] ||
> diff --git a/tg-export.sh b/tg-export.sh
> index 6d82d55..921e933 100644 tg-export.sh
> --- a/tg-export.sh
> +++ b/tg-export.sh
> @@ -57,8 +57,7 @@ if [ -z "$branches" ]; then
>  fi
>  
>  
> -playground="$(mktemp -d -t tg-export.XXXXXX)"
> -trap 'rm -rf "$playground"' EXIT
> +playground="$(get_temp tg-export -d)"
>  
>  
>  ## Collapse driver
> diff --git a/tg-info.sh b/tg-info.sh
> index 7d6a34c..10e257e 100644 tg-info.sh
> --- a/tg-info.sh
> +++ b/tg-info.sh
> @@ -51,7 +51,7 @@ fi
>  git cat-file blob "$name:.topdeps" |
>  	sed '1{ s/^/Depends: /; n; }; s/^/         /;'
>  
> -depcheck="$(mktemp -t tg-depcheck.XXXXXX)"
> +depcheck="$(get_temp tg-depcheck)"
>  missing_deps=
>  needs_update "$name" >"$depcheck" || :
>  if [ -n "$missing_deps" ]; then
> @@ -72,6 +72,5 @@ if [ -s "$depcheck" ]; then
>  else
>  	echo "Up-to-date."
>  fi
> -rm "$depcheck"
>  
>  # vim:noet
> diff --git a/tg-mail.sh b/tg-mail.sh
> index 8167ade..dd4a95a 100644 tg-mail.sh
> --- a/tg-mail.sh
> +++ b/tg-mail.sh
> @@ -34,7 +34,7 @@ if [ -n "$in_reply_to" ]; then
>  fi
>  
>  
> -patchfile="$(mktemp -t tg-mail.XXXXXX)"
> +patchfile="$(get_temp tg-mail)"
>  
>  $tg patch "$name" >"$patchfile"
>  
> @@ -54,6 +54,4 @@ people=
>  # NOTE: git-send-email handles cc itself
>  eval git send-email $send_email_args "$people" "$patchfile"
>  
> -rm "$patchfile"
> -
>  # vim:noet
> diff --git a/tg-patch.sh b/tg-patch.sh
> index 7bafdfe..68efcf0 100644 tg-patch.sh
> --- a/tg-patch.sh
> +++ b/tg-patch.sh
> @@ -51,7 +51,7 @@ echo
>  [ -n "$(git grep $diff_opts '^[-]--' ${diff_committed_only:+"$name"} -- ".topmsg")" ] || echo '---'
>  
>  # Evil obnoxious hack to work around the lack of git diff --exclude
> -git_is_stupid="$(mktemp -t tg-patch-changes.XXXXXX)"
> +git_is_stupid="$(get_temp tg-patch-changes)"
>  git diff --name-only $diff_opts "$base_rev" ${diff_committed_only:+"$name"} -- |
>  	fgrep -vx ".topdeps" |
>  	fgrep -vx ".topmsg" >"$git_is_stupid" || : # fgrep likes to fail randomly?
> @@ -61,7 +61,6 @@ if [ -s "$git_is_stupid" ]; then
>  else
>  	echo "No changes."
>  fi
> -rm "$git_is_stupid"
>  
>  echo '-- '
>  echo "tg: ($base_rev..) $name (depends on: $(cat_file "$topic:.topdeps" | paste -s -d' '))"
> diff --git a/tg-push.sh b/tg-push.sh
> index 199d738..a928fba 100644 tg-push.sh
> --- a/tg-push.sh
> +++ b/tg-push.sh
> @@ -45,8 +45,7 @@ for name in $branches; do
>  	ref_exists "$name" || die "detached HEAD? Can't push $name"
>  done
>  
> -_listfile="$(mktemp -t tg-push-listfile.XXXXXX)"
> -trap "rm -f \"$_listfile\"" 0
> +_listfile="$(get_temp tg-push-listfile)"
>  
>  push_branch()
>  {
> diff --git a/tg-summary.sh b/tg-summary.sh
> index af16888..612bd27 100644 tg-summary.sh
> --- a/tg-summary.sh
> +++ b/tg-summary.sh
> @@ -55,10 +55,9 @@ EOT
>  fi
>  
>  if [ -n "$sort" ]; then
> -	tsort_input=`mktemp`
> +	tsort_input="$(get_temp tg-summary-sort)"
>  	exec 4>$tsort_input
>  	exec 5<$tsort_input
> -	rm $tsort_input
>  fi
>  
>  ## List branches
> diff --git a/tg-update.sh b/tg-update.sh
> index b256c7c..5162c52 100644 tg-update.sh
> --- a/tg-update.sh
> +++ b/tg-update.sh
> @@ -27,7 +27,7 @@ base_rev="$(git rev-parse --short --verify "refs/top-bases/$name" 2>/dev/null)"
>  
>  ## First, take care of our base
>  
> -depcheck="$(mktemp -t tg-depcheck.XXXXXX)"
> +depcheck="$(get_temp tg-depcheck)"
>  missing_deps=
>  needs_update "$name" >"$depcheck" || :
>  [ -z "$missing_deps" ] || die "some dependencies are missing: $missing_deps"
> @@ -96,7 +96,6 @@ if [ -s "$depcheck" ]; then
>  else
>  	info "The base is up-to-date."
>  fi
> -rm "$depcheck"
>  
>  # Home, sweet home...
>  # (We want to always switch back, in case we were on the base from failed
> diff --git a/tg.sh b/tg.sh
> index 8264a3b..3805eeb 100644 tg.sh
> --- a/tg.sh
> +++ b/tg.sh
> @@ -150,7 +150,7 @@ recurse_deps()
>  	_name="$1"; # no shift
>  	_depchain="$*"
>  
> -	_depsfile="$(mktemp -t tg-depsfile.XXXXXX)"
> +	_depsfile="$(get_temp tg-depsfile)"
>  	# If no_remotes is unset check also our base against remote base.
>  	# Checking our head against remote head has to be done in the helper.
>  	if test -z "$no_remotes" && has_remote "top-bases/$_name"; then
> @@ -183,7 +183,6 @@ recurse_deps()
>  		eval "$_cmd"
>  	done <"$_depsfile"
>  	missing_deps="${missing_deps# }"
> -	rm "$_depsfile"
>  	return $_ret
>  }
>  
> @@ -334,19 +333,28 @@ setup_pager()
>  	# now spawn pager
>  	export LESS="${LESS:-FRSX}"	# as in pager.c:pager_preexec()
>  
> -	_pager_fifo_dir="$(mktemp -t -d tg-pager-fifo.XXXXXX)"
> -	_pager_fifo="$_pager_fifo_dir/0"
> -	mkfifo -m 600 "$_pager_fifo"
> +	# setup_pager should be called only once per command
> +	pager_fifo="$tg_tmp_dir/pager"
> +	mkfifo -m 600 "$pager_fifo"
>  
> -	"$TG_PAGER" < "$_pager_fifo" &
> -	exec > "$_pager_fifo"		# dup2(pager_fifo.in, 1)
> +	"$TG_PAGER" < "$pager_fifo" &
> +	exec > "$pager_fifo"		# dup2(pager_fifo.in, 1)
>  
>  	# this is needed so e.g. `git diff` will still colorize it's output if
>  	# requested in ~/.gitconfig with color.diff=auto
>  	export GIT_PAGER_IN_USE=1
>  
>  	# atexit(close(1); wait pager)
> -	trap "exec >&-; rm \"$_pager_fifo\"; rmdir \"$_pager_fifo_dir\"; wait" EXIT
> +	# deliberately overwrites the global EXIT trap
> +	trap "exec >&-; rm -rf \"$tg_tmp_dir\"; wait" EXIT
> +}
> +
> +# get_temp NAME [-d]
I like your patch in general, but would prefer to have
	# get_temp [-d] NAME
Show 5 quoted lines
> +# creates a new temporary file (or directory with -d) in the global
> +# temporary directory $tg_tmp_dir with pattern prefix NAME
> +get_temp()
> +{
> +	mktemp ${2-} "$tg_tmp_dir/$1.XXXXXX"
Does the - makes any difference?  If yes, is it portable (enough)?

Best regards Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Bert Wesarg· Oct 10, 2010, 07:47 UTC · re: Uwe Kleine-König · lore

Re: [TopGit PATCHv2] provide global temporary directory

2010/10/9 Uwe Kleine-König <u.kleine-koenig@pengutronix.de>:
Show 5 quoted lines
> On Sat, Oct 09, 2010 at 11:41:47PM +0200, Bert Wesarg wrote:
>> +# get_temp NAME [-d]
> I like your patch in general, but would prefer to have
>
>        # get_temp [-d] NAME
But that would complicate the argument evaluation, wouldn't it:
get_temp()
{
    if [ $# -eq 2 ]; then
        mktemp $1 "$tg_tmp_dir/$2.XXXXXX"
    else
        mktemp "$tg_tmp_dir/$1.XXXXXX"
    fi
}

This and the functions with [-i | -w] are internal and won't be called by the user, so we have full control over the calling convention and should make it easy for us. And putting the optional argument at the end is easy. I should probably remove the die call in cat_file, because that would be an programmer error not an user error.

Show 7 quoted lines
>
>> +# creates a new temporary file (or directory with -d) in the global
>> +# temporary directory $tg_tmp_dir with pattern prefix NAME
>> +get_temp()
>> +{
>> +     mktemp ${2-} "$tg_tmp_dir/$1.XXXXXX"
> Does the - makes any difference?  If yes, is it portable (enough)?

For me its an indicator, that it doesn't matter whether $2 was given or not. Also you could run under set -u without error. But I don't do this.

It's in POSIX [1].
Bert
[1] Statement given without interpretation.
Show 7 quoted lines
> Best regards
> Uwe
>
> --
> Pengutronix e.K.                           | Uwe Kleine-König            |
> Industrial Linux Solutions                 | http://www.pengutronix.de/  |
>

← back to recent threads