[RFD] git stash over OpenBSD/Linux NFS - cp -p breakage

[RFD] git stash over OpenBSD/Linux NFS - cp -p breakage

[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?

Re: [RFD] git stash over OpenBSD/Linux NFS - cp -p breakage

[Johannes Sixt]

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"

Re: [RFD] git stash over OpenBSD/Linux NFS - cp -p breakage

[Jakob Pfender]

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(-)

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

[PATCH] stash: copy the index using --index-output instead of cp -p

[Johannes Sixt]

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(-)

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

Re: [PATCH] stash: copy the index using --index-output instead of cp -p

[Junio C Hamano]

Johannes Sixt <j.sixt@viscovery.net> 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.

Re: [PATCH] stash: copy the index using --index-output instead of cp -p

[Bert Wesarg]

On Tue, Mar 15, 2011 at 20:28, Junio C Hamano <gitster@pobox.com> wrote:
> Johannes Sixt <j.sixt@viscovery.net> 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=<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

Re: [PATCH] stash: copy the index using --index-output instead of cp -p

[Johannes Sixt]

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

[PATCH 1/2] stash: fix incorrect quoting in cleanup of temporary files

[Johannes Sixt]

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(-)

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

[PATCH 2/2 v2] stash: copy the index using --index-output instead of cp -p

[Johannes Sixt]

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(-)

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