[PATCH] test: fix t7001 cp to use POSIX options

[PATCH] test: fix t7001 cp to use POSIX options

[Kyle J. McKay]

Since 11502468 and 04c1ee57 (both first appearing in v1.8.5), the
t7001-mv test has used "cp -a" to perform a copy in several of the
tests.

However, the "-a" option is not required for a POSIX cp utility and
some platforms' cp utilities do not support it.

The POSIX equivalent of -a is -R -P -p.

Change "cp -a" to "cp -R -P -p" so that the t7001-mv test works
on systems with a cp utility that only implements the POSIX
required set of options and not the "-a" option.

Signed-off-by: Kyle J. McKay <mackyle@gmail.com>

---
 t/t7001-mv.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/t/t7001-mv.sh b/t/t7001-mv.sh
index 215d43d6..c8ff9115 100755
--- a/t/t7001-mv.sh
+++ b/t/t7001-mv.sh
@@ -308,7 +308,7 @@ test_expect_success 'git mv moves a submodule with a .git directory and no .gitm
 	(
 		cd sub &&
 		rm -f .git &&
-		cp -a ../.git/modules/sub .git &&
+		cp -R -P -p ../.git/modules/sub .git &&
 		GIT_WORK_TREE=. git config --unset core.worktree
 	) &&
 	mkdir mod &&
@@ -331,7 +331,7 @@ test_expect_success 'git mv moves a submodule with a .git directory and .gitmodu
 	(
 		cd sub &&
 		rm -f .git &&
-		cp -a ../.git/modules/sub .git &&
+		cp -R -P -p ../.git/modules/sub .git &&
 		GIT_WORK_TREE=. git config --unset core.worktree
 	) &&
 	mkdir mod &&
-- 
tg: (0bc85abb..) t/t7001-posix-cp (depends on: maint)

Re: [PATCH] test: fix t7001 cp to use POSIX options

[Jeff King]

On Fri, Apr 11, 2014 at 01:24:02AM -0700, Kyle J. McKay wrote:

> Since 11502468 and 04c1ee57 (both first appearing in v1.8.5), the
> t7001-mv test has used "cp -a" to perform a copy in several of the
> tests.
> 
> However, the "-a" option is not required for a POSIX cp utility and
> some platforms' cp utilities do not support it.
> 
> The POSIX equivalent of -a is -R -P -p.
> 
> Change "cp -a" to "cp -R -P -p" so that the t7001-mv test works
> on systems with a cp utility that only implements the POSIX
> required set of options and not the "-a" option.

I wonder if the "-R" is the part that we actually care about here.
Including the others does not hurt in that case, but using only "-R"
would perhaps make it more obvious to a later reader of the code exactly
what we are trying to do.

-Peff

Re: [PATCH] test: fix t7001 cp to use POSIX options

[Kyle J. McKay]

On Apr 11, 2014, at 04:43, Jeff King wrote:
> On Fri, Apr 11, 2014 at 01:24:02AM -0700, Kyle J. McKay wrote:
>
>> Since 11502468 and 04c1ee57 (both first appearing in v1.8.5), the
>> t7001-mv test has used "cp -a" to perform a copy in several of the
>> tests.
>>
>> However, the "-a" option is not required for a POSIX cp utility and
>> some platforms' cp utilities do not support it.
>>
>> The POSIX equivalent of -a is -R -P -p.
>>
>> Change "cp -a" to "cp -R -P -p" so that the t7001-mv test works
>> on systems with a cp utility that only implements the POSIX
>> required set of options and not the "-a" option.
>
> I wonder if the "-R" is the part that we actually care about here.
> Including the others does not hurt in that case, but using only "-R"
> would perhaps make it more obvious to a later reader of the code  
> exactly
> what we are trying to do.

I was wondering the same thing myself, but Jens is on the Cc: list and  
added both of those, so I'm hoping he'll pipe in here about that.  I  
did notice that the other test scripts seem to only use -R, so that  
would definitely be a more consistent change to match the rest of the  
tests.

In any case v2 of the patch with just -R is attached below.  It seems
to pass the tests so it's probably fine.

--Kyle

---- 8< ----

Subject: [PATCH v2] test: fix t7001 cp to use POSIX options

Since 11502468 and 04c1ee57 (both first appearing in v1.8.5), the
t7001-mv test has used "cp -a" to perform a copy in several of the
tests.

However, the "-a" option is not required for a POSIX cp utility and
some platforms' cp utilities do not support it.

The POSIX equivalent of -a is -R -P -p, but the only option we
actually care about for the test is -R.

Change "cp -a" to "cp -R" so that the t7001-mv test works
on systems with a cp utility that only implements the POSIX
required set of options and not the "-a" option.

Signed-off-by: Kyle J. McKay <mackyle@gmail.com>

---
 t/t7001-mv.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/t/t7001-mv.sh b/t/t7001-mv.sh
index 215d43d6..675ca5bd 100755
--- a/t/t7001-mv.sh
+++ b/t/t7001-mv.sh
@@ -308,7 +308,7 @@ test_expect_success 'git mv moves a submodule with a .git directory and no .gitm
 	(
 		cd sub &&
 		rm -f .git &&
-		cp -a ../.git/modules/sub .git &&
+		cp -R ../.git/modules/sub .git &&
 		GIT_WORK_TREE=. git config --unset core.worktree
 	) &&
 	mkdir mod &&
@@ -331,7 +331,7 @@ test_expect_success 'git mv moves a submodule with a .git directory and .gitmodu
 	(
 		cd sub &&
 		rm -f .git &&
-		cp -a ../.git/modules/sub .git &&
+		cp -R ../.git/modules/sub .git &&
 		GIT_WORK_TREE=. git config --unset core.worktree
 	) &&
 	mkdir mod &&
-- 
tg: (0bc85abb..) t/t7001-posix-cp (depends on: maint)

Re: [PATCH] test: fix t7001 cp to use POSIX options

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Fri, Apr 11, 2014 at 01:24:02AM -0700, Kyle J. McKay wrote:
>
>> Since 11502468 and 04c1ee57 (both first appearing in v1.8.5), the
>> t7001-mv test has used "cp -a" to perform a copy in several of the
>> tests.
>> 
>> However, the "-a" option is not required for a POSIX cp utility and
>> some platforms' cp utilities do not support it.
>> 
>> The POSIX equivalent of -a is -R -P -p.
>> 
>> Change "cp -a" to "cp -R -P -p" so that the t7001-mv test works
>> on systems with a cp utility that only implements the POSIX
>> required set of options and not the "-a" option.
>
> I wonder if the "-R" is the part that we actually care about here.
> Including the others does not hurt in that case, but using only "-R"
> would perhaps make it more obvious to a later reader of the code exactly
> what we are trying to do.

These calls to "cp" are about "We know that earlier 'update' created
the GIT_DIR of the submodule in the top-level superproject, because
we are running a modern Git.  But we want to make sure the command
we are testing, "git mv", would work correctly if the repository
were made with an older Git that created the GIT_DIR embedded in the
working tree of the submodule, so let's emulate that case."  As we
create files and directories in GIT_DIR with explicit permission
bits, we may care about

 (1) making sure "git mv" can actually move the directory, with some
     paths having mode bits different from the umasked default; and

 (2) checking that the GIT_DIR after "git mv" has the permission
     bits right.

and if we cared, "-R -p" would be required here, not just "-R".

If core.prefersymlinkrefs is set, GIT_DIR would have a symbolic link
HEAD pointing at the current branch, and "-P" may become relevant,
but manually running "cp -R .git git && ls -l git/HEAD" in such an
old repository tells me that symbolic link HEAD is not dereferenced
without an explicit "-L", so I dunno.

Because we do not check anything inside GIT_DIR of the moved
submodule after "git mv" is done, the more correct use of "cp" is
moot for the purpose of (2), but it could be possible that "git mv"
fails to move a submodule with GIT_DIR created embedded in its
working tree by an older version of Git, while successfully copying
an emulated one, due to differences such as modes and symlinks.

The current implementation just does rename(2) on the whole
submodule working tree and let its contents move together, so I do
not think it matters at the moment for the purpose of (1); use of
flags other than "-R" are purely for future-proofing, I would think.

Re: [PATCH] test: fix t7001 cp to use POSIX options

[Jens Lehmann]

Am 11.04.2014 21:23, schrieb Junio C Hamano:
> Jeff King <peff@peff.net> writes:
> 
>> On Fri, Apr 11, 2014 at 01:24:02AM -0700, Kyle J. McKay wrote:
>>
>>> Since 11502468 and 04c1ee57 (both first appearing in v1.8.5), the
>>> t7001-mv test has used "cp -a" to perform a copy in several of the
>>> tests.
>>>
>>> However, the "-a" option is not required for a POSIX cp utility and
>>> some platforms' cp utilities do not support it.
>>>
>>> The POSIX equivalent of -a is -R -P -p.
>>>
>>> Change "cp -a" to "cp -R -P -p" so that the t7001-mv test works
>>> on systems with a cp utility that only implements the POSIX
>>> required set of options and not the "-a" option.
>>
>> I wonder if the "-R" is the part that we actually care about here.
>> Including the others does not hurt in that case, but using only "-R"
>> would perhaps make it more obvious to a later reader of the code exactly
>> what we are trying to do.
> 
> These calls to "cp" are about "We know that earlier 'update' created
> the GIT_DIR of the submodule in the top-level superproject, because
> we are running a modern Git.  But we want to make sure the command
> we are testing, "git mv", would work correctly if the repository
> were made with an older Git that created the GIT_DIR embedded in the
> working tree of the submodule, so let's emulate that case."  As we
> create files and directories in GIT_DIR with explicit permission
> bits, we may care about
> 
>  (1) making sure "git mv" can actually move the directory, with some
>      paths having mode bits different from the umasked default; and
> 
>  (2) checking that the GIT_DIR after "git mv" has the permission
>      bits right.

When writing these tests I didn't care about (2), but - in addition
to the first half of (1) - I had another thing in mind:

(3) "git mv" shouldn't try to update the 'core.worktree' setting
    when the GIT_DIR is embedded in the submodule's work tree.

> and if we cared, "-R -p" would be required here, not just "-R".
> 
> If core.prefersymlinkrefs is set, GIT_DIR would have a symbolic link
> HEAD pointing at the current branch, and "-P" may become relevant,
> but manually running "cp -R .git git && ls -l git/HEAD" in such an
> old repository tells me that symbolic link HEAD is not dereferenced
> without an explicit "-L", so I dunno.
> 
> Because we do not check anything inside GIT_DIR of the moved
> submodule after "git mv" is done, the more correct use of "cp" is
> moot for the purpose of (2), but it could be possible that "git mv"
> fails to move a submodule with GIT_DIR created embedded in its
> working tree by an older version of Git, while successfully copying
> an emulated one, due to differences such as modes and symlinks.
> 
> The current implementation just does rename(2) on the whole
> submodule working tree and let its contents move together, so I do
> not think it matters at the moment for the purpose of (1); use of
> flags other than "-R" are purely for future-proofing, I would think.

Thanks for your detailed analysis and sorry to all parties involved
for the hassle caused by my knee-jerk reaction to just use "cp -a"
when I wanted to have an exact copy of 'that' directory 'there'.

Given that all other tests just use "cp -R" too in that situation
I'm all for the second version of Kyle's patch, so an "Acked-by"
from me on that one.