threads / patch / 36388

patchtest: fix t7001 cp to use POSIX options

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

## tl;dr

5 messages between Apr 11, 2014 and Apr 12, 2014. Diffs are folded; open one to read it.

replies: 4people: 4as markdown or json

Kyle J. McKay· Apr 11, 2014, 08:24 UTC · lore

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

Show changes to t/t7001-mv.sh +2 −2

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)

Jeff King· Apr 11, 2014, 11:43 UTC · re: Kyle J. McKay · lore

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

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

Show 12 quoted lines

> 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

Kyle J. McKay· Apr 11, 2014, 13:44 UTC · re: Jeff King · lore

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

On Apr 11, 2014, at 04:43, Jeff King wrote:

Show 20 quoted lines

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

Show changes to t/t7001-mv.sh +2 −2

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)

Junio C Hamano· Apr 11, 2014, 19:23 UTC · re: Jeff King · lore

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

Jeff King <peff@peff.net> writes:

Show 19 quoted lines

> 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.

Jens Lehmann· Apr 12, 2014, 21:52 UTC · re: Junio C Hamano · lore

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

Am 11.04.2014 21:23, schrieb Junio C Hamano:

Show 36 quoted lines

> 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.

Show 19 quoted lines

> 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.