{"thread":{"id":"36388","subject":"[PATCH] test: fix t7001 cp to use POSIX options","startedAt":"2014-04-11T08:24:02Z","lastAt":"2014-04-12T21:52:35Z","messageCount":5,"participants":["Kyle J. McKay","Jeff King","Junio C Hamano","Jens Lehmann"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"238671","messageId":"1c3e86191de8c91545ac3ddc18fd31e@74d39fa044aa309eaea14b9f57fe79c","threadId":"36388","inReplyTo":null,"subject":"[PATCH] test: fix t7001 cp to use POSIX options","fromName":"Kyle J. McKay","fromEmail":"mackyle@gmail.com","sentAt":"2014-04-11T08:24:02Z","receivedAt":"2014-04-11T08:24:02Z","isPatch":true,"sender":{"key":"mackyle@gmail.com","avatar":"https://avatars.githubusercontent.com/u/813346?v=4"},"body":"Since 11502468 and 04c1ee57 (both first appearing in v1.8.5), the\nt7001-mv test has used \"cp -a\" to perform a copy in several of the\ntests.\n\nHowever, the \"-a\" option is not required for a POSIX cp utility and\nsome platforms' cp utilities do not support it.\n\nThe POSIX equivalent of -a is -R -P -p.\n\nChange \"cp -a\" to \"cp -R -P -p\" so that the t7001-mv test works\non systems with a cp utility that only implements the POSIX\nrequired set of options and not the \"-a\" option.\n\nSigned-off-by: Kyle J. McKay <mackyle@gmail.com>\n\n---\n t/t7001-mv.sh | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/t/t7001-mv.sh b/t/t7001-mv.sh\nindex 215d43d6..c8ff9115 100755\n--- a/t/t7001-mv.sh\n+++ b/t/t7001-mv.sh\n@@ -308,7 +308,7 @@ test_expect_success 'git mv moves a submodule with a .git directory and no .gitm\n \t(\n \t\tcd sub &&\n \t\trm -f .git &&\n-\t\tcp -a ../.git/modules/sub .git &&\n+\t\tcp -R -P -p ../.git/modules/sub .git &&\n \t\tGIT_WORK_TREE=. git config --unset core.worktree\n \t) &&\n \tmkdir mod &&\n@@ -331,7 +331,7 @@ test_expect_success 'git mv moves a submodule with a .git directory and .gitmodu\n \t(\n \t\tcd sub &&\n \t\trm -f .git &&\n-\t\tcp -a ../.git/modules/sub .git &&\n+\t\tcp -R -P -p ../.git/modules/sub .git &&\n \t\tGIT_WORK_TREE=. git config --unset core.worktree\n \t) &&\n \tmkdir mod &&\n-- \ntg: (0bc85abb..) t/t7001-posix-cp (depends on: maint)\n"},{"id":"238683","messageId":"20140411114338.GD28858@sigill.intra.peff.net","threadId":"36388","inReplyTo":"1c3e86191de8c91545ac3ddc18fd31e@74d39fa044aa309eaea14b9f57fe79c","subject":"Re: [PATCH] test: fix t7001 cp to use POSIX options","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-04-11T11:43:38Z","receivedAt":"2014-04-11T11:43:38Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Apr 11, 2014 at 01:24:02AM -0700, Kyle J. McKay wrote:\n\n> Since 11502468 and 04c1ee57 (both first appearing in v1.8.5), the\n> t7001-mv test has used \"cp -a\" to perform a copy in several of the\n> tests.\n> \n> However, the \"-a\" option is not required for a POSIX cp utility and\n> some platforms' cp utilities do not support it.\n> \n> The POSIX equivalent of -a is -R -P -p.\n> \n> Change \"cp -a\" to \"cp -R -P -p\" so that the t7001-mv test works\n> on systems with a cp utility that only implements the POSIX\n> required set of options and not the \"-a\" option.\n\nI wonder if the \"-R\" is the part that we actually care about here.\nIncluding the others does not hurt in that case, but using only \"-R\"\nwould perhaps make it more obvious to a later reader of the code exactly\nwhat we are trying to do.\n\n-Peff\n"},{"id":"238693","messageId":"4def160d638b09b02e04f45688c41a8@74d39fa044aa309eaea14b9f57fe79c","threadId":"36388","inReplyTo":"20140411114338.GD28858@sigill.intra.peff.net","subject":"Re: [PATCH] test: fix t7001 cp to use POSIX options","fromName":"Kyle J. McKay","fromEmail":"mackyle@gmail.com","sentAt":"2014-04-11T13:44:07Z","receivedAt":"2014-04-11T13:44:07Z","isPatch":true,"sender":{"key":"mackyle@gmail.com","avatar":"https://avatars.githubusercontent.com/u/813346?v=4"},"body":"On Apr 11, 2014, at 04:43, Jeff King wrote:\n> On Fri, Apr 11, 2014 at 01:24:02AM -0700, Kyle J. McKay wrote:\n>\n>> Since 11502468 and 04c1ee57 (both first appearing in v1.8.5), the\n>> t7001-mv test has used \"cp -a\" to perform a copy in several of the\n>> tests.\n>>\n>> However, the \"-a\" option is not required for a POSIX cp utility and\n>> some platforms' cp utilities do not support it.\n>>\n>> The POSIX equivalent of -a is -R -P -p.\n>>\n>> Change \"cp -a\" to \"cp -R -P -p\" so that the t7001-mv test works\n>> on systems with a cp utility that only implements the POSIX\n>> required set of options and not the \"-a\" option.\n>\n> I wonder if the \"-R\" is the part that we actually care about here.\n> Including the others does not hurt in that case, but using only \"-R\"\n> would perhaps make it more obvious to a later reader of the code  \n> exactly\n> what we are trying to do.\n\nI was wondering the same thing myself, but Jens is on the Cc: list and  \nadded both of those, so I'm hoping he'll pipe in here about that.  I  \ndid notice that the other test scripts seem to only use -R, so that  \nwould definitely be a more consistent change to match the rest of the  \ntests.\n\nIn any case v2 of the patch with just -R is attached below.  It seems\nto pass the tests so it's probably fine.\n\n--Kyle\n\n---- 8< ----\n\nSubject: [PATCH v2] test: fix t7001 cp to use POSIX options\n\nSince 11502468 and 04c1ee57 (both first appearing in v1.8.5), the\nt7001-mv test has used \"cp -a\" to perform a copy in several of the\ntests.\n\nHowever, the \"-a\" option is not required for a POSIX cp utility and\nsome platforms' cp utilities do not support it.\n\nThe POSIX equivalent of -a is -R -P -p, but the only option we\nactually care about for the test is -R.\n\nChange \"cp -a\" to \"cp -R\" so that the t7001-mv test works\non systems with a cp utility that only implements the POSIX\nrequired set of options and not the \"-a\" option.\n\nSigned-off-by: Kyle J. McKay <mackyle@gmail.com>\n\n---\n t/t7001-mv.sh | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/t/t7001-mv.sh b/t/t7001-mv.sh\nindex 215d43d6..675ca5bd 100755\n--- a/t/t7001-mv.sh\n+++ b/t/t7001-mv.sh\n@@ -308,7 +308,7 @@ test_expect_success 'git mv moves a submodule with a .git directory and no .gitm\n \t(\n \t\tcd sub &&\n \t\trm -f .git &&\n-\t\tcp -a ../.git/modules/sub .git &&\n+\t\tcp -R ../.git/modules/sub .git &&\n \t\tGIT_WORK_TREE=. git config --unset core.worktree\n \t) &&\n \tmkdir mod &&\n@@ -331,7 +331,7 @@ test_expect_success 'git mv moves a submodule with a .git directory and .gitmodu\n \t(\n \t\tcd sub &&\n \t\trm -f .git &&\n-\t\tcp -a ../.git/modules/sub .git &&\n+\t\tcp -R ../.git/modules/sub .git &&\n \t\tGIT_WORK_TREE=. git config --unset core.worktree\n \t) &&\n \tmkdir mod &&\n-- \ntg: (0bc85abb..) t/t7001-posix-cp (depends on: maint)\n"},{"id":"238732","messageId":"xmqq1tx3u0ts.fsf@gitster.dls.corp.google.com","threadId":"36388","inReplyTo":"20140411114338.GD28858@sigill.intra.peff.net","subject":"Re: [PATCH] test: fix t7001 cp to use POSIX options","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-04-11T19:23:59Z","receivedAt":"2014-04-11T19:23:59Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> On Fri, Apr 11, 2014 at 01:24:02AM -0700, Kyle J. McKay wrote:\n>\n>> Since 11502468 and 04c1ee57 (both first appearing in v1.8.5), the\n>> t7001-mv test has used \"cp -a\" to perform a copy in several of the\n>> tests.\n>> \n>> However, the \"-a\" option is not required for a POSIX cp utility and\n>> some platforms' cp utilities do not support it.\n>> \n>> The POSIX equivalent of -a is -R -P -p.\n>> \n>> Change \"cp -a\" to \"cp -R -P -p\" so that the t7001-mv test works\n>> on systems with a cp utility that only implements the POSIX\n>> required set of options and not the \"-a\" option.\n>\n> I wonder if the \"-R\" is the part that we actually care about here.\n> Including the others does not hurt in that case, but using only \"-R\"\n> would perhaps make it more obvious to a later reader of the code exactly\n> what we are trying to do.\n\nThese calls to \"cp\" are about \"We know that earlier 'update' created\nthe GIT_DIR of the submodule in the top-level superproject, because\nwe are running a modern Git.  But we want to make sure the command\nwe are testing, \"git mv\", would work correctly if the repository\nwere made with an older Git that created the GIT_DIR embedded in the\nworking tree of the submodule, so let's emulate that case.\"  As we\ncreate files and directories in GIT_DIR with explicit permission\nbits, we may care about\n\n (1) making sure \"git mv\" can actually move the directory, with some\n     paths having mode bits different from the umasked default; and\n\n (2) checking that the GIT_DIR after \"git mv\" has the permission\n     bits right.\n\nand if we cared, \"-R -p\" would be required here, not just \"-R\".\n\nIf core.prefersymlinkrefs is set, GIT_DIR would have a symbolic link\nHEAD pointing at the current branch, and \"-P\" may become relevant,\nbut manually running \"cp -R .git git && ls -l git/HEAD\" in such an\nold repository tells me that symbolic link HEAD is not dereferenced\nwithout an explicit \"-L\", so I dunno.\n\nBecause we do not check anything inside GIT_DIR of the moved\nsubmodule after \"git mv\" is done, the more correct use of \"cp\" is\nmoot for the purpose of (2), but it could be possible that \"git mv\"\nfails to move a submodule with GIT_DIR created embedded in its\nworking tree by an older version of Git, while successfully copying\nan emulated one, due to differences such as modes and symlinks.\n\nThe current implementation just does rename(2) on the whole\nsubmodule working tree and let its contents move together, so I do\nnot think it matters at the moment for the purpose of (1); use of\nflags other than \"-R\" are purely for future-proofing, I would think.\n"},{"id":"238798","messageId":"5349B5A3.30601@web.de","threadId":"36388","inReplyTo":"xmqq1tx3u0ts.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] test: fix t7001 cp to use POSIX options","fromName":"Jens Lehmann","fromEmail":"jens.lehmann@web.de","sentAt":"2014-04-12T21:52:35Z","receivedAt":"2014-04-12T21:52:35Z","isPatch":true,"sender":{"key":"jens.lehmann@web.de","avatar":"https://avatars.githubusercontent.com/u/135220?v=4"},"body":"Am 11.04.2014 21:23, schrieb Junio C Hamano:\n> Jeff King <peff@peff.net> writes:\n> \n>> On Fri, Apr 11, 2014 at 01:24:02AM -0700, Kyle J. McKay wrote:\n>>\n>>> Since 11502468 and 04c1ee57 (both first appearing in v1.8.5), the\n>>> t7001-mv test has used \"cp -a\" to perform a copy in several of the\n>>> tests.\n>>>\n>>> However, the \"-a\" option is not required for a POSIX cp utility and\n>>> some platforms' cp utilities do not support it.\n>>>\n>>> The POSIX equivalent of -a is -R -P -p.\n>>>\n>>> Change \"cp -a\" to \"cp -R -P -p\" so that the t7001-mv test works\n>>> on systems with a cp utility that only implements the POSIX\n>>> required set of options and not the \"-a\" option.\n>>\n>> I wonder if the \"-R\" is the part that we actually care about here.\n>> Including the others does not hurt in that case, but using only \"-R\"\n>> would perhaps make it more obvious to a later reader of the code exactly\n>> what we are trying to do.\n> \n> These calls to \"cp\" are about \"We know that earlier 'update' created\n> the GIT_DIR of the submodule in the top-level superproject, because\n> we are running a modern Git.  But we want to make sure the command\n> we are testing, \"git mv\", would work correctly if the repository\n> were made with an older Git that created the GIT_DIR embedded in the\n> working tree of the submodule, so let's emulate that case.\"  As we\n> create files and directories in GIT_DIR with explicit permission\n> bits, we may care about\n> \n>  (1) making sure \"git mv\" can actually move the directory, with some\n>      paths having mode bits different from the umasked default; and\n> \n>  (2) checking that the GIT_DIR after \"git mv\" has the permission\n>      bits right.\n\nWhen writing these tests I didn't care about (2), but - in addition\nto the first half of (1) - I had another thing in mind:\n\n(3) \"git mv\" shouldn't try to update the 'core.worktree' setting\n    when the GIT_DIR is embedded in the submodule's work tree.\n\n> and if we cared, \"-R -p\" would be required here, not just \"-R\".\n> \n> If core.prefersymlinkrefs is set, GIT_DIR would have a symbolic link\n> HEAD pointing at the current branch, and \"-P\" may become relevant,\n> but manually running \"cp -R .git git && ls -l git/HEAD\" in such an\n> old repository tells me that symbolic link HEAD is not dereferenced\n> without an explicit \"-L\", so I dunno.\n> \n> Because we do not check anything inside GIT_DIR of the moved\n> submodule after \"git mv\" is done, the more correct use of \"cp\" is\n> moot for the purpose of (2), but it could be possible that \"git mv\"\n> fails to move a submodule with GIT_DIR created embedded in its\n> working tree by an older version of Git, while successfully copying\n> an emulated one, due to differences such as modes and symlinks.\n> \n> The current implementation just does rename(2) on the whole\n> submodule working tree and let its contents move together, so I do\n> not think it matters at the moment for the purpose of (1); use of\n> flags other than \"-R\" are purely for future-proofing, I would think.\n\nThanks for your detailed analysis and sorry to all parties involved\nfor the hassle caused by my knee-jerk reaction to just use \"cp -a\"\nwhen I wanted to have an exact copy of 'that' directory 'there'.\n\nGiven that all other tests just use \"cp -R\" too in that situation\nI'm all for the second version of Kyle's patch, so an \"Acked-by\"\nfrom me on that one.\n"}]}