[RFD] 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

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=<tmp-file>, which copies all the data including the timestamp and avoids using cp -p altogether.

Signed-off-by: Johannes Sixt <j.sixt@viscovery.net>
Acked-by: Jakob Pfender <jpfender@elegosoft.com>
---
  git-stash.sh |    3 +--
  1 files changed, 1 insertions(+), 2 deletions(-)

From: Johannes Sixt <j6t@kdbg.org>

'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 <j6t@kdbg.org>
Tested-by: Jakob Pfender <jpfender@elegosoft.com>
---
 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(-)

Johannes Sixt <j.sixt@viscovery.net> writes:

Thanks, will apply.

On Tue, Mar 15, 2011 at 20:28, Junio C Hamano <gitster@pobox.com> wrote:

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 <j6t@kdbg.org>

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 <j6t@kdbg.org>
---
 git-stash.sh |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

From: Johannes Sixt <j6t@kdbg.org>

'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 <j6t@kdbg.org>
---
 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(-)