{"thread":{"id":"47475","subject":"[PATCH 1/4] t/lib-submodule-update.sh: clarify test","startedAt":"2017-12-20T22:17:40Z","lastAt":"2017-12-22T20:01:14Z","messageCount":7,"participants":["Stefan Beller","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":4},"messages":[{"id":"335110","messageId":"20171220221725.129162-2-sbeller@google.com","threadId":"47475","inReplyTo":"20171220221725.129162-1-sbeller@google.com","subject":"[PATCH 1/4] t/lib-submodule-update.sh: clarify test","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-12-20T22:17:22Z","receivedAt":"2017-12-20T22:17:40Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Keep the local branch name as the upstream branch name to avoid confusion.\n\nSigned-off-by: Stefan Beller <sbeller@google.com>\n---\n t/lib-submodule-update.sh | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/t/lib-submodule-update.sh b/t/lib-submodule-update.sh\nindex 38dadd2c29..d7699046f6 100755\n--- a/t/lib-submodule-update.sh\n+++ b/t/lib-submodule-update.sh\n@@ -664,8 +664,8 @@ test_submodule_recursing_with_args_common() {\n \t\t\tcd submodule_update &&\n \t\t\tgit -C sub1 checkout -b keep_branch &&\n \t\t\tgit -C sub1 rev-parse HEAD >expect &&\n-\t\t\tgit branch -t check-keep origin/modify_sub1 &&\n-\t\t\t$command check-keep &&\n+\t\t\tgit branch -t modify_sub1 origin/modify_sub1 &&\n+\t\t\t$command modify_sub1 &&\n \t\t\ttest_superproject_content origin/modify_sub1 &&\n \t\t\ttest_submodule_content sub1 origin/modify_sub1 &&\n \t\t\tgit -C sub1 rev-parse keep_branch >actual &&\n-- \n2.15.1.620.gb9897f4670-goog\n\n"},{"id":"335111","messageId":"20171220221725.129162-3-sbeller@google.com","threadId":"47475","inReplyTo":"20171220221725.129162-1-sbeller@google.com","subject":"[PATCH 2/4] t/lib-submodule-update.sh: Fix test ignoring ignored files in submodules","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-12-20T22:17:23Z","receivedAt":"2017-12-20T22:17:41Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"It turns out that this buggy test passed due to the buggy implementation,\nwhich will soon be corrected.  Let's fix the test first.\n\nSigned-off-by: Stefan Beller <sbeller@google.com>\n---\n t/lib-submodule-update.sh | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/t/lib-submodule-update.sh b/t/lib-submodule-update.sh\nindex d7699046f6..fb0173ea87 100755\n--- a/t/lib-submodule-update.sh\n+++ b/t/lib-submodule-update.sh\n@@ -885,6 +885,7 @@ test_submodule_switch_recursing_with_args () {\n \t\t(\n \t\t\tcd submodule_update &&\n \t\t\tgit branch -t replace_sub1_with_file origin/replace_sub1_with_file &&\n+\t\t\techo ignored >.git/modules/sub1/info/exclude &&\n \t\t\t: >sub1/ignored &&\n \t\t\t$command replace_sub1_with_file &&\n \t\t\ttest_superproject_content origin/replace_sub1_with_file &&\n-- \n2.15.1.620.gb9897f4670-goog\n\n"},{"id":"335112","messageId":"20171220221725.129162-4-sbeller@google.com","threadId":"47475","inReplyTo":"20171220221725.129162-1-sbeller@google.com","subject":"[PATCH 3/4] unpack-trees: Fix same() for submodules","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-12-20T22:17:24Z","receivedAt":"2017-12-20T22:17:43Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"The function same(a, b) is used to check if two entries a and b are the\nsame.  As the index contains the staged files the same() function can be\nused to check if files between a given revision and the index are the same.\n\nIn case of submodules, the gitlink entry is showing up as not modified\ndespite potential changes inside the submodule.\n\nFix the function to examine submodules by looking inside the submodule.\nThis patch alone doesn't affect any correctness garantuees, but in\ncombination with the next patch this fixes the new test introduced\nearlier in this series.\n\nThis may be a slight performance regression as now we have to\ninspect any submodule thouroughly.\n\nSigned-off-by: Stefan Beller <sbeller@google.com>\n---\n unpack-trees.c | 2 ++\n 1 file changed, 2 insertions(+)\n\ndiff --git a/unpack-trees.c b/unpack-trees.c\nindex bf8b602901..4d839e8edb 100644\n--- a/unpack-trees.c\n+++ b/unpack-trees.c\n@@ -1427,6 +1427,8 @@ static int same(const struct cache_entry *a, const struct cache_entry *b)\n \t\treturn 1;\n \tif ((a->ce_flags | b->ce_flags) & CE_CONFLICTED)\n \t\treturn 0;\n+\tif (S_ISGITLINK(b->ce_mode) && should_update_submodules())\n+\t\treturn !oidcmp(&a->oid, &b->oid) && !is_submodule_modified(b->name, 0);\n \treturn a->ce_mode == b->ce_mode &&\n \t       !oidcmp(&a->oid, &b->oid);\n }\n-- \n2.15.1.620.gb9897f4670-goog\n\n"},{"id":"335113","messageId":"20171220221725.129162-5-sbeller@google.com","threadId":"47475","inReplyTo":"20171220221725.129162-1-sbeller@google.com","subject":"[PATCH 4/4] submodule: submodule_move_head omits old argument in forced case","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-12-20T22:17:25Z","receivedAt":"2017-12-20T22:17:45Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"When using hard reset or forced checkout with the option to recurse into\nsubmodules, the submodules need to be reset, too.\n\nIt turns out that we need to omit the duplicate old argument to read-tree\nin all forced cases to omit the 2 way merge and use the more assertive\nbehavior of reading the specific new tree into the index and updating\nthe working tree.\n\nSigned-off-by: Stefan Beller <sbeller@google.com>\n---\n submodule.c               |  4 +++-\n t/lib-submodule-update.sh | 11 +++++++++++\n 2 files changed, 14 insertions(+), 1 deletion(-)\n\ndiff --git a/submodule.c b/submodule.c\nindex fa25888783..db0f7ac51e 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -1653,7 +1653,9 @@ int submodule_move_head(const char *path,\n \telse\n \t\targv_array_push(&cp.args, \"-m\");\n \n-\targv_array_push(&cp.args, old ? old : EMPTY_TREE_SHA1_HEX);\n+\tif (!(flags & SUBMODULE_MOVE_HEAD_FORCE))\n+\t\targv_array_push(&cp.args, old ? old : EMPTY_TREE_SHA1_HEX);\n+\n \targv_array_push(&cp.args, new ? new : EMPTY_TREE_SHA1_HEX);\n \n \tif (run_command(&cp)) {\ndiff --git a/t/lib-submodule-update.sh b/t/lib-submodule-update.sh\nindex fb0173ea87..380ef4b4ae 100755\n--- a/t/lib-submodule-update.sh\n+++ b/t/lib-submodule-update.sh\n@@ -1015,4 +1015,15 @@ test_submodule_forced_switch_recursing_with_args () {\n \t\t\ttest_submodule_content sub1 origin/modify_sub1\n \t\t)\n \t'\n+\n+\ttest_expect_success \"$command: changed submodule worktree is reset\" '\n+\t\tprolog &&\n+\t\treset_work_tree_to_interested add_sub1 &&\n+\t\t(\n+\t\t\tcd submodule_update &&\n+\t\t\trm sub1/file1 &&\n+\t\t\t$command HEAD &&\n+\t\t\ttest_path_is_file sub1/file1\n+\t\t)\n+\t'\n }\n-- \n2.15.1.620.gb9897f4670-goog\n\n"},{"id":"335114","messageId":"20171220221725.129162-1-sbeller@google.com","threadId":"47475","inReplyTo":null,"subject":"[PATCHv2 0/4] Fix --recurse-submodules for submodule worktree changes","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-12-20T22:17:21Z","receivedAt":"2017-12-20T22:19:31Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"v2:\n* squashed the test patch and the bugfix, rewriting the commit message entirely.\n  This might not the way Jonathan imagined how I address the potential user\n  confusion[1], but I think it does the job.\n\n[1] https://public-inbox.org/git/20171220000148.GJ240141@aiede.mtv.corp.google.com/\n\nv1:\n\nThe fix is in the last patch, the first patches are just massaging the code\nbase to make the fix easy.\n\nThe second patch fixes a bug in the test, which was ineffective at testing.\nThe third patch shows the problem this series addresses,\nthe fourth patch is a little refactoring, which I want to keep separate\nas I would expect it to be a performance regression[1].\nThe first patch is unrelated, but improves the readability of submodule test\ncases, which we'd want to improve further.\n\nThanks,\nStefan\n\n[1] The performance should improve once we don't use so many processes\n    any more, but that repository series is stalled for now.\n\n\nStefan Beller (4):\n  t/lib-submodule-update.sh: clarify test\n  t/lib-submodule-update.sh: Fix test ignoring ignored files in\n    submodules\n  unpack-trees: Fix same() for submodules\n  submodule: submodule_move_head omits old argument in forced case\n\n submodule.c               |  4 +++-\n t/lib-submodule-update.sh | 16 ++++++++++++++--\n unpack-trees.c            |  2 ++\n 3 files changed, 19 insertions(+), 3 deletions(-)\n\n-- \n2.15.1.620.gb9897f4670-goog\n\n"},{"id":"335185","messageId":"xmqq3742sfhv.fsf@gitster.mtv.corp.google.com","threadId":"47475","inReplyTo":"20171220221725.129162-4-sbeller@google.com","subject":"Re: [PATCH 3/4] unpack-trees: Fix same() for submodules","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-12-22T19:57:00Z","receivedAt":"2017-12-22T19:57:08Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller <sbeller@google.com> writes:\n\n> The function same(a, b) is used to check if two entries a and b are the\n> same.  As the index contains the staged files the same() function can be\n> used to check if files between a given revision and the index are the same.\n>\n> In case of submodules, the gitlink entry is showing up as not modified\n> despite potential changes inside the submodule.\n>\n> Fix the function to examine submodules by looking inside the submodule.\n> This patch alone doesn't affect any correctness garantuees, but in\n\nguarantees?  I somehow misread this as orangutan ;-)\n\n> combination with the next patch this fixes the new test introduced\n> earlier in this series.\n>\n> This may be a slight performance regression as now we have to\n> inspect any submodule thouroughly.\n>\n> Signed-off-by: Stefan Beller <sbeller@google.com>\n> ---\n>  unpack-trees.c | 2 ++\n>  1 file changed, 2 insertions(+)\n>\n> diff --git a/unpack-trees.c b/unpack-trees.c\n> index bf8b602901..4d839e8edb 100644\n> --- a/unpack-trees.c\n> +++ b/unpack-trees.c\n> @@ -1427,6 +1427,8 @@ static int same(const struct cache_entry *a, const struct cache_entry *b)\n>  \t\treturn 1;\n>  \tif ((a->ce_flags | b->ce_flags) & CE_CONFLICTED)\n>  \t\treturn 0;\n> +\tif (S_ISGITLINK(b->ce_mode) && should_update_submodules())\n> +\t\treturn !oidcmp(&a->oid, &b->oid) && !is_submodule_modified(b->name, 0);\n\nIsn't this is working at the wrong level?\n\nIt is technically correct to say \"the function is used to check if\ntwo entries a and be are the same\", but that statement misses a more\nimportant aspect of the function.  It is asked if these two things\nare the same and does not get involved in seeing if the working tree\nis dirty with respect to (one of) the entries it was passed.\n\nFor example, a virtual merge in a recursive merge would not (and\nshould not) care if local changes exist in the working tree, and\nthis function does not get any hint if it is to\nlook at the working tree to tell such a case and the outermost merge\napart.\n\nSomebody has to be careful not to stomp on local changes in the\noutmost merge in a recursive merge, but I do not think this function\nis designed to be the place to do so.  Isn't a submodule with\n\"potential changes\" the same way?\n\n\n\n>  \treturn a->ce_mode == b->ce_mode &&\n>  \t       !oidcmp(&a->oid, &b->oid);\n>  }\n"},{"id":"335186","messageId":"xmqqy3lur0qj.fsf@gitster.mtv.corp.google.com","threadId":"47475","inReplyTo":"20171220221725.129162-5-sbeller@google.com","subject":"Re: [PATCH 4/4] submodule: submodule_move_head omits old argument in forced case","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-12-22T20:01:08Z","receivedAt":"2017-12-22T20:01:14Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller <sbeller@google.com> writes:\n\n> When using hard reset or forced checkout with the option to recurse into\n> submodules, the submodules need to be reset, too.\n>\n> It turns out that we need to omit the duplicate old argument to read-tree\n> in all forced cases to omit the 2 way merge and use the more assertive\n> behavior of reading the specific new tree into the index and updating\n> the working tree.\n\nThe phrase \"the more assertive\" made me imagine something like\n\"reset --hard\", which resurrect lost paths and also get rid of added\npaths.  \"reading the specific new tree into the index\" smells more\nlike \"checkout $tree-ish $paths\" that has an overlay semantics, that\nresurrects lost paths but does not get rid of added paths.\n\nPerhaps not just \"rm sub1/file1\" but also add a new file that is not\nin HEAD to ensure that it will be blown away when $command is run\nto ensure that we got the distinction between the two right?\n\n>\n> Signed-off-by: Stefan Beller <sbeller@google.com>\n> ---\n>  submodule.c               |  4 +++-\n>  t/lib-submodule-update.sh | 11 +++++++++++\n>  2 files changed, 14 insertions(+), 1 deletion(-)\n>\n> diff --git a/submodule.c b/submodule.c\n> index fa25888783..db0f7ac51e 100644\n> --- a/submodule.c\n> +++ b/submodule.c\n> @@ -1653,7 +1653,9 @@ int submodule_move_head(const char *path,\n>  \telse\n>  \t\targv_array_push(&cp.args, \"-m\");\n>  \n> -\targv_array_push(&cp.args, old ? old : EMPTY_TREE_SHA1_HEX);\n> +\tif (!(flags & SUBMODULE_MOVE_HEAD_FORCE))\n> +\t\targv_array_push(&cp.args, old ? old : EMPTY_TREE_SHA1_HEX);\n> +\n>  \targv_array_push(&cp.args, new ? new : EMPTY_TREE_SHA1_HEX);\n>  \n>  \tif (run_command(&cp)) {\n> diff --git a/t/lib-submodule-update.sh b/t/lib-submodule-update.sh\n> index fb0173ea87..380ef4b4ae 100755\n> --- a/t/lib-submodule-update.sh\n> +++ b/t/lib-submodule-update.sh\n> @@ -1015,4 +1015,15 @@ test_submodule_forced_switch_recursing_with_args () {\n>  \t\t\ttest_submodule_content sub1 origin/modify_sub1\n>  \t\t)\n>  \t'\n> +\n> +\ttest_expect_success \"$command: changed submodule worktree is reset\" '\n> +\t\tprolog &&\n> +\t\treset_work_tree_to_interested add_sub1 &&\n> +\t\t(\n> +\t\t\tcd submodule_update &&\n> +\t\t\trm sub1/file1 &&\n> +\t\t\t$command HEAD &&\n> +\t\t\ttest_path_is_file sub1/file1\n> +\t\t)\n> +\t'\n>  }\n"}]}