git stash over OpenBSD/Linux NFS - cp -p breakage

We have an NFS setup with Linux machines mounting an NFS that is hosted on an OpenBSD server. Recently, we discovered git-stash breaking with:

$ git stash
cp: preserving permissions for `/home/jpfender/stashtest/.git/.git-stash.3056-index': Operation not supported
Cannot save the current worktree state

This was discovered to be caused by a bug in cp that causes 'cp -p' to fail in this particular NFS setup - preserving permissions in an NFS shared across Linux and OpenBSD machines doesn't work.

I looked at git-stash.sh and could not discover a reason why it had to use 'cp -p'. I patched it to use only cp without preserving permissions, and everything seemed to work fine. All stash tests succeeded (bar two known breakages).

So my question is: Does git-stash really need 'cp -p'? Is it safe to remove?

Am 3/15/2011 11:08, schrieb Jakob Pfender:

Yes. No.

The timestamp of the index file is important. It is needed to discover racily-clean index entries. Therefore, the -p must remain.

You can also try the patch below. Warning: completely untested.

-- Hannes

index 7561b37..fa62135 100755
@@ -82,10 +82,9 @@ create_stash () {
 		# state of the working tree
 		w_tree=$( (
 			rm -f "$TMP-index" &&
-			cp -p ${GIT_INDEX_FILE-"$GIT_DIR/index"} "$TMP-index" &&
+			git read-tree --index-output="$TMP-index" -m $i_tree &&
 			GIT_INDEX_FILE="$TMP-index" &&
 			export GIT_INDEX_FILE &&
-			git read-tree -m $i_tree &&
 			git diff --name-only -z HEAD | git update-index -z --add --remove --stdin &&
 			git write-tree &&
 			rm -f "$TMP-index"

You're right. The funny thing is: 'cp --preserve=timestamps' works over NFS, 'cp -p' just fails because it wants to copy file ownership as well.

Works fine for me! Also has the advantage that a git tool is used to copy the data, which can interpret the data better than cp could if need arose.

Does anyone want to commit Hannes' patch with the following log message:

Subject: [PATCH] git-stash.sh: Use git-read-tree instead of cp -p

cp -p fails in certain Linux/*BSD NFS setups because it wants to preserve file ownership. However, the reason we use -p is because we want to preserve the timestamp. This can be achieved by using git-read-tree --index-output=<tmp-file>, which copies all the data including the timestamp and avoids using cp -p altogether.

---
 git-stash.sh |    3 +--
 1 files changed, 1 insertions(+), 2 deletions(-)

index 7561b37..fa62135 100755
@@ -82,10 +82,9 @@ create_stash () {
 		# state of the working tree
 		w_tree=$( (
 			rm -f "$TMP-index" &&
-			cp -p ${GIT_INDEX_FILE-"$GIT_DIR/index"} "$TMP-index" &&
+			git read-tree --index-output="$TMP-index" -m $i_tree &&
 			GIT_INDEX_FILE="$TMP-index" &&
 			export GIT_INDEX_FILE &&
-			git read-tree -m $i_tree &&
 			git diff --name-only -z HEAD | git update-index -z --add --remove 
--stdin &&
 			git write-tree &&

rm -f "$TMP-index"

1.7.0.4

From: Johannes Sixt <[email protected]>

'git stash create' must operate with a temporary index. For this purpose, it used 'cp -p' to create a copy. -p is needed to preserve the timestamp of the index file. Now Jakob Pfender reported a certain combination of a Linux NFS client, OpenBSD NFS server, and cp implementation where this operation failed.

Luckily, the first operation in git-stash after copying the index is to call 'git read-tree'. Therefore, use --index-output instead of 'cp -p' to write the copy of the index.

---
Here's a wrap-up. Acked-by is reserved for people whose area-of-expertise is touched by someone else. Tested-by is more appropriate here.

 git-stash.sh |    3 +--
 1 files changed, 1 insertions(+), 2 deletions(-)

index 7561b37..fa62135 100755
@@ -82,10 +82,9 @@ create_stash () {
 		# state of the working tree
 		w_tree=$( (
 			rm -f "$TMP-index" &&
-			cp -p ${GIT_INDEX_FILE-"$GIT_DIR/index"} "$TMP-index" &&
+			git read-tree --index-output="$TMP-index" -m $i_tree &&
 			GIT_INDEX_FILE="$TMP-index" &&
 			export GIT_INDEX_FILE &&
-			git read-tree -m $i_tree &&
 			git diff --name-only -z HEAD | git update-index -z --add --remove --stdin &&
 			git write-tree &&
 			rm -f "$TMP-index"

1.7.4.rc3.76.gfb457d

Thanks, will apply.

Please note that the file to the --index-output option needs to be on the same filesystem as GIT_INDEX_FILE, so that it can be renamed. Here is the corresponding manual page paragraph for this:

--index-output=<file>
Instead of writing the results out to $GIT_INDEX_FILE, write the resulting index in the named file. While the command is operating, the original index file is locked with the same mechanism as usual. The file must allow to be rename(2)ed into from a temporary file that is created next to the usual index file; typically this means it needs to be on the same filesystem as the index file itself, and you need write permission to the directories the index file and index output file are located in.

I don' have the full context, so I can't judge whether $TMP-index is guaranteed to be on the same filesystem as ${GIT_INDEX_FILE-"$GIT_DIR/index"}.

Bert

Am 3/15/2011 20:53, schrieb Bert Wesarg:

Good catch. Here follows a mini-series that address this and also fixes some left-over temporary files.

-- Hannes

From: Johannes Sixt <[email protected]>

The * was inside the quotes, so that the pattern was never expanded and the temporary files were never removed. As a consequence, 'stash -p' left a .git-stash-*-patch file in $GIT_DIR. Other code paths did not leave files behind because they removed the temporary file themselves, at least in non-error paths.

---
 git-stash.sh |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

index 7561b37..7c0d563 100755
@@ -17,7 +17,7 @@ require_work_tree
 cd_to_toplevel
 
 TMP="$GIT_DIR/.git-stash.$$"
-trap 'rm -f "$TMP-*"' 0
+trap 'rm -f "$TMP-"*' 0
 
 ref_stash=refs/stash
 

1.7.4.1.316.g2ab2cb

From: Johannes Sixt <[email protected]>

'git stash create' must operate with a temporary index. For this purpose, it used 'cp -p' to create a copy. -p is needed to preserve the timestamp of the index file. Now Jakob Pfender reported a certain combination of a Linux NFS client, OpenBSD NFS server, and cp implementation where this operation failed.

Luckily, the first operation in git-stash after copying the index is to call 'git read-tree'. Therefore, use --index-output instead of 'cp -p' to write the copy of the index.

--index-output requires that the specified file is on the same volume as the source index, so that the lock file can be rename()d. For this reason, the name of the temporary index is constructed in a way different from the other temporary files. The code path of 'stash -p' also needs a temporary index, but we do not use the new name because it does not depend on the same precondition as --index-output.

---
Changes since v1:

- Addresses the --index-output precondition.

- Also removes the first 'rm -f "$TMP-index" &&'

Warning: This was only tested by the test suite.

 git-stash.sh |   11 +++++------
 1 files changed, 5 insertions(+), 6 deletions(-)

index 7c0d563..5130228 100755
@@ -17,7 +17,8 @@ require_work_tree
 cd_to_toplevel
 
 TMP="$GIT_DIR/.git-stash.$$"
-trap 'rm -f "$TMP-"*' 0
+TMPindex=${GIT_INDEX_FILE-"$GIT_DIR/index"}.stash.$$
+trap 'rm -f "$TMP-"* "$TMPindex"' 0
 
 ref_stash=refs/stash
 

@@ -81,14 +82,12 @@ create_stash () {
 
 		# state of the working tree
 		w_tree=$( (
-			rm -f "$TMP-index" &&
-			cp -p ${GIT_INDEX_FILE-"$GIT_DIR/index"} "$TMP-index" &&
-			GIT_INDEX_FILE="$TMP-index" &&
+			git read-tree --index-output="$TMPindex" -m $i_tree &&
+			GIT_INDEX_FILE="$TMPindex" &&
 			export GIT_INDEX_FILE &&
-			git read-tree -m $i_tree &&
 			git diff --name-only -z HEAD | git update-index -z --add --remove --stdin &&
 			git write-tree &&
-			rm -f "$TMP-index"
+			rm -f "$TMPindex"
 		) ) ||
 			die "Cannot save the current worktree state"
 

1.7.4.1.316.g2ab2cb