{"thread":{"id":"26737","subject":"[RFD] git stash over OpenBSD/Linux NFS - cp -p breakage","startedAt":"2011-03-15T10:08:36Z","lastAt":"2011-03-16T08:18:49Z","messageCount":9,"participants":["Jakob Pfender","Johannes Sixt","Junio C Hamano","Bert Wesarg"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"163375","messageId":"4D7F3AA4.1080202@elegosoft.com","threadId":"26737","inReplyTo":null,"subject":"[RFD] git stash over OpenBSD/Linux NFS - cp -p breakage","fromName":"Jakob Pfender","fromEmail":"jpfender@elegosoft.com","sentAt":"2011-03-15T10:08:36Z","receivedAt":"2011-03-15T10:08:36Z","isPatch":false,"sender":{"key":"jpfender@elegosoft.com","avatar":null},"body":"We have an NFS setup with Linux machines mounting an NFS that is hosted \non an OpenBSD server. Recently, we discovered git-stash breaking with:\n\n$ git stash\ncp: preserving permissions for \n`/home/jpfender/stashtest/.git/.git-stash.3056-index': Operation not \nsupported\nCannot save the current worktree state\n\nThis was discovered to be caused by a bug in cp that causes 'cp -p' to \nfail in this particular NFS setup - preserving permissions in an NFS \nshared across Linux and OpenBSD machines doesn't work.\n\nI looked at git-stash.sh and could not discover a reason why it had to \nuse 'cp -p'. I patched it to use only cp without preserving permissions, \nand everything seemed to work fine. All stash tests succeeded (bar two \nknown breakages).\n\nSo my question is: Does git-stash really need 'cp -p'? Is it safe to remove?\n"},{"id":"163380","messageId":"4D7F466A.4090508@viscovery.net","threadId":"26737","inReplyTo":"4D7F3AA4.1080202@elegosoft.com","subject":"Re: [RFD] git stash over OpenBSD/Linux NFS - cp -p breakage","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2011-03-15T10:58:50Z","receivedAt":"2011-03-15T10:58:50Z","isPatch":false,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 3/15/2011 11:08, schrieb Jakob Pfender:\n> We have an NFS setup with Linux machines mounting an NFS that is hosted on\n> an OpenBSD server. Recently, we discovered git-stash breaking with:\n> \n> $ git stash\n> cp: preserving permissions for\n> `/home/jpfender/stashtest/.git/.git-stash.3056-index': Operation not\n> supported\n> Cannot save the current worktree state\n> \n> This was discovered to be caused by a bug in cp that causes 'cp -p' to\n> fail in this particular NFS setup - preserving permissions in an NFS\n> shared across Linux and OpenBSD machines doesn't work.\n> \n> I looked at git-stash.sh and could not discover a reason why it had to use\n> 'cp -p'. I patched it to use only cp without preserving permissions, and\n> everything seemed to work fine. All stash tests succeeded (bar two known\n> breakages).\n> \n> So my question is: Does git-stash really need 'cp -p'? Is it safe to remove?\n\nYes. No.\n\nThe timestamp of the index file is important. It is needed to discover\nracily-clean index entries. Therefore, the -p must remain.\n\nYou can also try the patch below. Warning: completely untested.\n\n-- Hannes\n\ndiff --git a/git-stash.sh b/git-stash.sh\nindex 7561b37..fa62135 100755\n--- a/git-stash.sh\n+++ b/git-stash.sh\n@@ -82,10 +82,9 @@ create_stash () {\n \t\t# state of the working tree\n \t\tw_tree=$( (\n \t\t\trm -f \"$TMP-index\" &&\n-\t\t\tcp -p ${GIT_INDEX_FILE-\"$GIT_DIR/index\"} \"$TMP-index\" &&\n+\t\t\tgit read-tree --index-output=\"$TMP-index\" -m $i_tree &&\n \t\t\tGIT_INDEX_FILE=\"$TMP-index\" &&\n \t\t\texport GIT_INDEX_FILE &&\n-\t\t\tgit read-tree -m $i_tree &&\n \t\t\tgit diff --name-only -z HEAD | git update-index -z --add --remove --stdin &&\n \t\t\tgit write-tree &&\n \t\t\trm -f \"$TMP-index\"\n"},{"id":"163383","messageId":"4D7F54C2.5060500@elegosoft.com","threadId":"26737","inReplyTo":"4D7F466A.4090508@viscovery.net","subject":"Re: [RFD] git stash over OpenBSD/Linux NFS - cp -p breakage","fromName":"Jakob Pfender","fromEmail":"jpfender@elegosoft.com","sentAt":"2011-03-15T12:00:02Z","receivedAt":"2011-03-15T12:00:02Z","isPatch":false,"sender":{"key":"jpfender@elegosoft.com","avatar":null},"body":"\n\nOn 03/15/2011 11:58 AM, Johannes Sixt wrote:\n > Am 3/15/2011 11:08, schrieb Jakob Pfender:\n >> We have an NFS setup with Linux machines mounting an NFS that is \nhosted on\n >> an OpenBSD server. Recently, we discovered git-stash breaking with:\n >>\n >> $ git stash\n >> cp: preserving permissions for\n >> `/home/jpfender/stashtest/.git/.git-stash.3056-index': Operation not\n >> supported\n >> Cannot save the current worktree state\n >>\n >> This was discovered to be caused by a bug in cp that causes 'cp -p' to\n >> fail in this particular NFS setup - preserving permissions in an NFS\n >> shared across Linux and OpenBSD machines doesn't work.\n >>\n >> I looked at git-stash.sh and could not discover a reason why it had \nto use\n >> 'cp -p'. I patched it to use only cp without preserving permissions, and\n >> everything seemed to work fine. All stash tests succeeded (bar two known\n >> breakages).\n >>\n >> So my question is: Does git-stash really need 'cp -p'? Is it safe to \nremove?\n >\n > Yes. No.\n >\n > The timestamp of the index file is important. It is needed to discover\n > racily-clean index entries. Therefore, the -p must remain.\n\nYou're right. The funny thing is: 'cp --preserve=timestamps' works over \nNFS, 'cp -p' just fails because it wants to copy file ownership as well.\n\n >\n > You can also try the patch below. Warning: completely untested.\n\nWorks fine for me! Also has the advantage that a git tool is used to \ncopy the data, which can interpret the data better than cp could if need \narose.\n\nDoes anyone want to commit Hannes' patch with the following log message:\n\nSubject: [PATCH] git-stash.sh: Use git-read-tree instead of cp -p\n\ncp -p fails in certain Linux/*BSD NFS setups because it wants to\npreserve file ownership. However, the reason we use -p is because we\nwant to preserve the timestamp. This can be achieved by using\ngit-read-tree --index-output=, which copies all the data\nincluding the timestamp and avoids using cp -p altogether.\n\nSigned-off-by: Johannes Sixt \nAcked-by: Jakob Pfender \n---\n git-stash.sh | 3 +--\n 1 files changed, 1 insertions(+), 2 deletions(-)\n\ndiff --git a/git-stash.sh b/git-stash.sh\nindex 7561b37..fa62135 100755\n--- a/git-stash.sh\n+++ b/git-stash.sh\n@@ -82,10 +82,9 @@ create_stash () {\n \t\t# state of the working tree\n \t\tw_tree=$( (\n \t\t\trm -f \"$TMP-index\" &&\n-\t\t\tcp -p ${GIT_INDEX_FILE-\"$GIT_DIR/index\"} \"$TMP-index\" &&\n+\t\t\tgit read-tree --index-output=\"$TMP-index\" -m $i_tree &&\n \t\t\tGIT_INDEX_FILE=\"$TMP-index\" &&\n \t\t\texport GIT_INDEX_FILE &&\n-\t\t\tgit read-tree -m $i_tree &&\n \t\t\tgit diff --name-only -z HEAD | git update-index -z --add --remove \n--stdin &&\n \t\t\tgit write-tree &&\n \t\t\trm -f \"$TMP-index\"\n-- \n1.7.0.4\n"},{"id":"163385","messageId":"4D7F5DA0.3030608@viscovery.net","threadId":"26737","inReplyTo":"4D7F54C2.5060500@elegosoft.com","subject":"[PATCH] stash: copy the index using --index-output instead of cp -p","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2011-03-15T12:37:52Z","receivedAt":"2011-03-15T12:37:52Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"From: Johannes Sixt \n\n'git stash create' must operate with a temporary index. For this purpose,\nit used 'cp -p' to create a copy. -p is needed to preserve the timestamp\nof the index file. Now Jakob Pfender reported a certain combination of\na Linux NFS client, OpenBSD NFS server, and cp implementation where this\noperation failed.\n\nLuckily, the first operation in git-stash after copying the index is to\ncall 'git read-tree'. Therefore, use --index-output instead of 'cp -p'\nto write the copy of the index.\n\nSigned-off-by: Johannes Sixt \nTested-by: Jakob Pfender \n---\n Here's a wrap-up. Acked-by is reserved for people whose area-of-expertise\n is touched by someone else. Tested-by is more appropriate here.\n\n git-stash.sh | 3 +--\n 1 files changed, 1 insertions(+), 2 deletions(-)\n\ndiff --git a/git-stash.sh b/git-stash.sh\nindex 7561b37..fa62135 100755\n--- a/git-stash.sh\n+++ b/git-stash.sh\n@@ -82,10 +82,9 @@ create_stash () {\n \t\t# state of the working tree\n \t\tw_tree=$( (\n \t\t\trm -f \"$TMP-index\" &&\n-\t\t\tcp -p ${GIT_INDEX_FILE-\"$GIT_DIR/index\"} \"$TMP-index\" &&\n+\t\t\tgit read-tree --index-output=\"$TMP-index\" -m $i_tree &&\n \t\t\tGIT_INDEX_FILE=\"$TMP-index\" &&\n \t\t\texport GIT_INDEX_FILE &&\n-\t\t\tgit read-tree -m $i_tree &&\n \t\t\tgit diff --name-only -z HEAD | git update-index -z --add --remove --stdin &&\n \t\t\tgit write-tree &&\n \t\t\trm -f \"$TMP-index\"\n-- \n1.7.4.rc3.76.gfb457d\n"},{"id":"163404","messageId":"7vk4g09o9x.fsf@alter.siamese.dyndns.org","threadId":"26737","inReplyTo":"4D7F5DA0.3030608@viscovery.net","subject":"Re: [PATCH] stash: copy the index using --index-output instead of cp -p","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-03-15T19:28:26Z","receivedAt":"2011-03-15T19:28:26Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Sixt writes:\n\n> Luckily, the first operation in git-stash after copying the index is to\n> call 'git read-tree'. Therefore, use --index-output instead of 'cp -p'\n> to write the copy of the index.\n\nThanks, will apply.\n"},{"id":"163406","messageId":"AANLkTi=+-tDz6XSSUAyvzGZ6fJohSU+rY4GhRa5C-+oA@mail.gmail.com","threadId":"26737","inReplyTo":"7vk4g09o9x.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] stash: copy the index using --index-output instead of cp -p","fromName":"Bert Wesarg","fromEmail":"bert.wesarg@googlemail.com","sentAt":"2011-03-15T19:53:08Z","receivedAt":"2011-03-15T19:53:08Z","isPatch":true,"sender":{"key":"bert.wesarg@googlemail.com","avatar":"https://avatars.githubusercontent.com/u/111934?v=4"},"body":"On Tue, Mar 15, 2011 at 20:28, Junio C Hamano wrote:\n> Johannes Sixt writes:\n>\n>> Luckily, the first operation in git-stash after copying the index is to\n>> call 'git read-tree'. Therefore, use --index-output instead of 'cp -p'\n>> to write the copy of the index.\n>\n> Thanks, will apply.\n\nPlease note that the file to the --index-output option needs to be on\nthe same filesystem as GIT_INDEX_FILE, so that it can be renamed. Here\nis the corresponding manual page paragraph for this:\n\n --index-output=\n Instead of writing the results out to $GIT_INDEX_FILE, write the\n resulting index in the named file. While the command is operating,\n the original index file is locked with the same mechanism as usual.\n The file must allow to be rename(2)ed into from a temporary file\n that is created next to the usual index file; typically this means\n it needs to be on the same filesystem as the index file itself, and\n you need write permission to the directories the index file and\n index output file are located in.\n\nI don' have the full context, so I can't judge whether $TMP-index is\nguaranteed to be on the same filesystem as\n${GIT_INDEX_FILE-\"$GIT_DIR/index\"}.\n\nBert\n"},{"id":"163436","messageId":"4D8070E1.8000305@viscovery.net","threadId":"26737","inReplyTo":"AANLkTi=+-tDz6XSSUAyvzGZ6fJohSU+rY4GhRa5C-+oA@mail.gmail.com","subject":"Re: [PATCH] stash: copy the index using --index-output instead of cp -p","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2011-03-16T08:12:17Z","receivedAt":"2011-03-16T08:12:17Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 3/15/2011 20:53, schrieb Bert Wesarg:\n> Please note that the file to the --index-output option needs to be on\n> the same filesystem as GIT_INDEX_FILE, so that it can be renamed.\n\nGood catch. Here follows a mini-series that address this and also fixes\nsome left-over temporary files.\n\n-- Hannes\n"},{"id":"163437","messageId":"4D807169.5070804@viscovery.net","threadId":"26737","inReplyTo":"4D8070E1.8000305@viscovery.net","subject":"[PATCH 1/2] stash: fix incorrect quoting in cleanup of temporary files","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2011-03-16T08:14:33Z","receivedAt":"2011-03-16T08:14:33Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"From: Johannes Sixt \n\nThe * was inside the quotes, so that the pattern was never expanded and the\ntemporary files were never removed. As a consequence, 'stash -p' left a\n.git-stash-*-patch file in $GIT_DIR. Other code paths did not leave files\nbehind because they removed the temporary file themselves, at least in\nnon-error paths.\n\nSigned-off-by: Johannes Sixt \n---\n git-stash.sh | 2 +-\n 1 files changed, 1 insertions(+), 1 deletions(-)\n\ndiff --git a/git-stash.sh b/git-stash.sh\nindex 7561b37..7c0d563 100755\n--- a/git-stash.sh\n+++ b/git-stash.sh\n@@ -17,7 +17,7 @@ require_work_tree\n cd_to_toplevel\n \n TMP=\"$GIT_DIR/.git-stash.$$\"\n-trap 'rm -f \"$TMP-*\"' 0\n+trap 'rm -f \"$TMP-\"*' 0\n \n ref_stash=refs/stash\n \n-- \n1.7.4.1.316.g2ab2cb\n"},{"id":"163438","messageId":"4D807269.1070802@viscovery.net","threadId":"26737","inReplyTo":"4D8070E1.8000305@viscovery.net","subject":"[PATCH 2/2 v2] stash: copy the index using --index-output instead of cp -p","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2011-03-16T08:18:49Z","receivedAt":"2011-03-16T08:18:49Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"From: Johannes Sixt \n\n'git stash create' must operate with a temporary index. For this purpose,\nit used 'cp -p' to create a copy. -p is needed to preserve the timestamp\nof the index file. Now Jakob Pfender reported a certain combination of\na Linux NFS client, OpenBSD NFS server, and cp implementation where this\noperation failed.\n\nLuckily, the first operation in git-stash after copying the index is to\ncall 'git read-tree'. Therefore, use --index-output instead of 'cp -p'\nto write the copy of the index.\n\n--index-output requires that the specified file is on the same volume as\nthe source index, so that the lock file can be rename()d. For this reason,\nthe name of the temporary index is constructed in a way different from the\nother temporary files. The code path of 'stash -p' also needs a temporary\nindex, but we do not use the new name because it does not depend on the\nsame precondition as --index-output.\n\nSigned-off-by: Johannes Sixt \n---\n Changes since v1:\n\n - Addresses the --index-output precondition.\n\n - Also removes the first 'rm -f \"$TMP-index\" &&'\n\n Warning: This was only tested by the test suite.\n\n git-stash.sh | 11 +++++------\n 1 files changed, 5 insertions(+), 6 deletions(-)\n\ndiff --git a/git-stash.sh b/git-stash.sh\nindex 7c0d563..5130228 100755\n--- a/git-stash.sh\n+++ b/git-stash.sh\n@@ -17,7 +17,8 @@ require_work_tree\n cd_to_toplevel\n \n TMP=\"$GIT_DIR/.git-stash.$$\"\n-trap 'rm -f \"$TMP-\"*' 0\n+TMPindex=${GIT_INDEX_FILE-\"$GIT_DIR/index\"}.stash.$$\n+trap 'rm -f \"$TMP-\"* \"$TMPindex\"' 0\n \n ref_stash=refs/stash\n \n@@ -81,14 +82,12 @@ create_stash () {\n \n \t\t# state of the working tree\n \t\tw_tree=$( (\n-\t\t\trm -f \"$TMP-index\" &&\n-\t\t\tcp -p ${GIT_INDEX_FILE-\"$GIT_DIR/index\"} \"$TMP-index\" &&\n-\t\t\tGIT_INDEX_FILE=\"$TMP-index\" &&\n+\t\t\tgit read-tree --index-output=\"$TMPindex\" -m $i_tree &&\n+\t\t\tGIT_INDEX_FILE=\"$TMPindex\" &&\n \t\t\texport GIT_INDEX_FILE &&\n-\t\t\tgit read-tree -m $i_tree &&\n \t\t\tgit diff --name-only -z HEAD | git update-index -z --add --remove --stdin &&\n \t\t\tgit write-tree &&\n-\t\t\trm -f \"$TMP-index\"\n+\t\t\trm -f \"$TMPindex\"\n \t\t) ) ||\n \t\t\tdie \"Cannot save the current worktree state\"\n \n-- \n1.7.4.1.316.g2ab2cb\n"}]}