# [RFD] git stash over OpenBSD/Linux NFS - cp -p breakage 9 messages from 2011-03-15 to 2011-03-16. Participants: Jakob Pfender, Johannes Sixt, Junio C Hamano, Bert Wesarg. Thread: https://gitlist.dev/t/26737 ## Jakob Pfender, 2011-03-15 10:08 Subject: [RFD] git stash over OpenBSD/Linux NFS - cp -p breakage Message-ID: <4D7F3AA4.1080202@elegosoft.com> URL: https://gitlist.dev/e/4D7F3AA4.1080202%40elegosoft.com ``` 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? ``` ## Johannes Sixt, 2011-03-15 10:58 Subject: Re: [RFD] git stash over OpenBSD/Linux NFS - cp -p breakage Message-ID: <4D7F466A.4090508@viscovery.net> URL: https://gitlist.dev/e/4D7F466A.4090508%40viscovery.net In-Reply-To: <4D7F3AA4.1080202@elegosoft.com> ``` Am 3/15/2011 11:08, schrieb Jakob Pfender: > 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? 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 diff --git a/git-stash.sh b/git-stash.sh index 7561b37..fa62135 100755 --- a/git-stash.sh +++ b/git-stash.sh @@ -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" ``` ## Jakob Pfender, 2011-03-15 12:00 Subject: Re: [RFD] git stash over OpenBSD/Linux NFS - cp -p breakage Message-ID: <4D7F54C2.5060500@elegosoft.com> URL: https://gitlist.dev/e/4D7F54C2.5060500%40elegosoft.com In-Reply-To: <4D7F466A.4090508@viscovery.net> ``` On 03/15/2011 11:58 AM, Johannes Sixt wrote: > Am 3/15/2011 11:08, schrieb Jakob Pfender: >> 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? > > 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'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. > > You can also try the patch below. Warning: completely untested. 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=, which copies all the data including the timestamp and avoids using cp -p altogether. Signed-off-by: Johannes Sixt Acked-by: Jakob Pfender --- git-stash.sh | 3 +-- 1 files changed, 1 insertions(+), 2 deletions(-) diff --git a/git-stash.sh b/git-stash.sh index 7561b37..fa62135 100755 --- a/git-stash.sh +++ b/git-stash.sh @@ -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 ``` ## Johannes Sixt, 2011-03-15 12:37 Subject: [PATCH] stash: copy the index using --index-output instead of cp -p Message-ID: <4D7F5DA0.3030608@viscovery.net> URL: https://gitlist.dev/e/4D7F5DA0.3030608%40viscovery.net In-Reply-To: <4D7F54C2.5060500@elegosoft.com> ``` From: Johannes Sixt '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. Signed-off-by: Johannes Sixt Tested-by: Jakob Pfender --- 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(-) diff --git a/git-stash.sh b/git-stash.sh index 7561b37..fa62135 100755 --- a/git-stash.sh +++ b/git-stash.sh @@ -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 ``` ## Junio C Hamano, 2011-03-15 19:28 Subject: Re: [PATCH] stash: copy the index using --index-output instead of cp -p Message-ID: <7vk4g09o9x.fsf@alter.siamese.dyndns.org> URL: https://gitlist.dev/e/7vk4g09o9x.fsf%40alter.siamese.dyndns.org In-Reply-To: <4D7F5DA0.3030608@viscovery.net> ``` Johannes Sixt writes: > 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. Thanks, will apply. ``` ## Bert Wesarg, 2011-03-15 19:53 Subject: Re: [PATCH] stash: copy the index using --index-output instead of cp -p Message-ID: URL: https://gitlist.dev/e/AANLkTi%3D%2B-tDz6XSSUAyvzGZ6fJohSU%2BrY4GhRa5C-%2BoA%40mail.gmail.com In-Reply-To: <7vk4g09o9x.fsf@alter.siamese.dyndns.org> ``` On Tue, Mar 15, 2011 at 20:28, Junio C Hamano wrote: > Johannes Sixt writes: > >> 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. > > 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= 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 ``` ## Johannes Sixt, 2011-03-16 08:12 Subject: Re: [PATCH] stash: copy the index using --index-output instead of cp -p Message-ID: <4D8070E1.8000305@viscovery.net> URL: https://gitlist.dev/e/4D8070E1.8000305%40viscovery.net In-Reply-To: ``` Am 3/15/2011 20:53, schrieb Bert Wesarg: > 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. Good catch. Here follows a mini-series that address this and also fixes some left-over temporary files. -- Hannes ``` ## Johannes Sixt, 2011-03-16 08:14 Subject: [PATCH 1/2] stash: fix incorrect quoting in cleanup of temporary files Message-ID: <4D807169.5070804@viscovery.net> URL: https://gitlist.dev/e/4D807169.5070804%40viscovery.net In-Reply-To: <4D8070E1.8000305@viscovery.net> ``` From: Johannes Sixt 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. Signed-off-by: Johannes Sixt --- git-stash.sh | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/git-stash.sh b/git-stash.sh index 7561b37..7c0d563 100755 --- a/git-stash.sh +++ b/git-stash.sh @@ -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 ``` ## Johannes Sixt, 2011-03-16 08:18 Subject: [PATCH 2/2 v2] stash: copy the index using --index-output instead of cp -p Message-ID: <4D807269.1070802@viscovery.net> URL: https://gitlist.dev/e/4D807269.1070802%40viscovery.net In-Reply-To: <4D8070E1.8000305@viscovery.net> ``` From: Johannes Sixt '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. Signed-off-by: Johannes Sixt --- 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(-) diff --git a/git-stash.sh b/git-stash.sh index 7c0d563..5130228 100755 --- a/git-stash.sh +++ b/git-stash.sh @@ -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 ```