{"thread":{"id":"47468","subject":"[PATCH 0/5] Fix --recurse-submodules for submodule worktree changes","startedAt":"2017-12-19T22:26:53Z","lastAt":"2018-01-09T22:45:22Z","messageCount":38,"participants":["Stefan Beller","Jonathan Nieder","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":5},"messages":[{"id":"335048","messageId":"20171219222636.216001-1-sbeller@google.com","threadId":"47468","inReplyTo":null,"subject":"[PATCH 0/5] Fix --recurse-submodules for submodule worktree changes","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-12-19T22:26:31Z","receivedAt":"2017-12-19T22:26:53Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"The 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\nStefan Beller (5):\n t/lib-submodule-update.sh: clarify test\n t/lib-submodule-update.sh: Fix test ignoring ignored files in\n submodules\n t/lib-submodule-update.sh: add new test for submodule internal change\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":"335049","messageId":"20171219222636.216001-2-sbeller@google.com","threadId":"47468","inReplyTo":"20171219222636.216001-1-sbeller@google.com","subject":"[PATCH 1/5] t/lib-submodule-update.sh: clarify test","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-12-19T22:26:32Z","receivedAt":"2017-12-19T22:26:56Z","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 \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":"335050","messageId":"20171219222636.216001-4-sbeller@google.com","threadId":"47468","inReplyTo":"20171219222636.216001-1-sbeller@google.com","subject":"[PATCH 3/5] t/lib-submodule-update.sh: add new test for submodule internal change","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-12-19T22:26:34Z","receivedAt":"2017-12-19T22:27:00Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"The test is marked as a failure as the fix comes in a later patch.\n\nSigned-off-by: Stefan Beller \n---\n t/lib-submodule-update.sh | 11 +++++++++++\n 1 file changed, 11 insertions(+)\n\ndiff --git a/t/lib-submodule-update.sh b/t/lib-submodule-update.sh\nindex fb0173ea87..15cf3e0b8b 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_failure \"$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":"335051","messageId":"20171219222636.216001-3-sbeller@google.com","threadId":"47468","inReplyTo":"20171219222636.216001-1-sbeller@google.com","subject":"[PATCH 2/5] t/lib-submodule-update.sh: Fix test ignoring ignored files in submodules","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-12-19T22:26:33Z","receivedAt":"2017-12-19T22:27:01Z","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 \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":"335052","messageId":"20171219222636.216001-5-sbeller@google.com","threadId":"47468","inReplyTo":"20171219222636.216001-1-sbeller@google.com","subject":"[PATCH 4/5] unpack-trees: Fix same() for submodules","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-12-19T22:26:35Z","receivedAt":"2017-12-19T22:27:02Z","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 \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":"335053","messageId":"20171219222636.216001-6-sbeller@google.com","threadId":"47468","inReplyTo":"20171219222636.216001-1-sbeller@google.com","subject":"[PATCH 5/5] submodule: submodule_move_head omits old argument in forced case","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-12-19T22:26:36Z","receivedAt":"2017-12-19T22:27:08Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"With the previous patch applied (fix of the same() function), the\nfunction `submodule_move_head` may be invoked with the same argument\nfor the `old` and `new` state of a submodule, for example when you\nrun `reset --hard --recurse-submodules` in the superproject that has no\nchange in the gitlink entry, but only worktree related change in the\nsubmodule. The read-tree call in the submodule is not amused about\nthe duplicate argument.\n\nIt turns out that we can omit the duplicate old argument in all forced\ncases anyway, so let's do that.\n\nSigned-off-by: Stefan Beller \n---\n submodule.c | 4 +++-\n t/lib-submodule-update.sh | 2 +-\n 2 files changed, 4 insertions(+), 2 deletions(-)\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 15cf3e0b8b..7b6661cc84 100755\n--- a/t/lib-submodule-update.sh\n+++ b/t/lib-submodule-update.sh\n@@ -1016,7 +1016,7 @@ test_submodule_forced_switch_recursing_with_args () {\n \t\t)\n \t'\n \n-\ttest_expect_failure \"$command: changed submodule worktree is reset\" '\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-- \n2.15.1.620.gb9897f4670-goog\n\n"},{"id":"335054","messageId":"20171219223130.GF240141@aiede.mtv.corp.google.com","threadId":"47468","inReplyTo":"20171219222636.216001-4-sbeller@google.com","subject":"Re: [PATCH 3/5] t/lib-submodule-update.sh: add new test for submodule internal change","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2017-12-19T22:31:30Z","receivedAt":"2017-12-19T22:31:39Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nStefan Beller wrote:\n\n> The test is marked as a failure as the fix comes in a later patch.\n>\n> Signed-off-by: Stefan Beller \n> ---\n> t/lib-submodule-update.sh | 11 +++++++++++\n> 1 file changed, 11 insertions(+)\n\nI think I'd find this easier to undrestand if it were squashed with the\npatch that fixes it.\n\nThis is part of test_submodule_foced_switch --- does that mean it\naffects both checkout -f and reset --hard, or only the latter?\n\nThanks,\nJonathan\n"},{"id":"335056","messageId":"CAGZ79kZhiX8jqz-LxpeMp92caqU6urxS3T8gxoB4toDkGw0g6w@mail.gmail.com","threadId":"47468","inReplyTo":"20171219223130.GF240141@aiede.mtv.corp.google.com","subject":"Re: [PATCH 3/5] t/lib-submodule-update.sh: add new test for submodule internal change","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-12-19T22:35:02Z","receivedAt":"2017-12-19T22:35:08Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Tue, Dec 19, 2017 at 2:31 PM, Jonathan Nieder wrote:\n> Hi,\n>\n> Stefan Beller wrote:\n>\n>> The test is marked as a failure as the fix comes in a later patch.\n>>\n>> Signed-off-by: Stefan Beller \n>> ---\n>> t/lib-submodule-update.sh | 11 +++++++++++\n>> 1 file changed, 11 insertions(+)\n>\n> I think I'd find this easier to undrestand if it were squashed with the\n> patch that fixes it.\n\nok, let's squash this into the last patch then.\n\n> This is part of test_submodule_foced_switch --- does that mean it\n> affects both checkout -f and reset --hard, or only the latter?\n\nAll of\n checkout -f\n reset --hard (and not reset --keep ;)\n read-tree -u --reset\n\nThanks,\nStefan\n"},{"id":"335057","messageId":"20171219224431.GG240141@aiede.mtv.corp.google.com","threadId":"47468","inReplyTo":"20171219222636.216001-6-sbeller@google.com","subject":"Re: [PATCH 5/5] submodule: submodule_move_head omits old argument in forced case","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2017-12-19T22:44:31Z","receivedAt":"2017-12-19T22:44:40Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nI had trouble understanding what this fixes, so I'll try nitpicking a\nbit as a sideways way to address that.\n\nStefan Beller wrote:\n\n> With the previous patch applied (fix of the same() function),\n\nThis tripped me up a bit. Usually commits assume that all previous\npatches have already been applied, since that allows the maintainer to\napply the early part of a series on one day and the later part another\nday without losing too much context.\n\nI think this intends to say something like\n\n\tNow that we allow recursing into an unchanged submodule (see\n\t\"unpack-trees: Fix same() for submodules\", 2017-12-19), it is\n\tpossible for ...\n\n> the\n> function `submodule_move_head` may be invoked with the same argument\n> for the `old` and `new` state of a submodule, for example when you\n> run `reset --hard --recurse-submodules` in the superproject that has no\n> change in the gitlink entry, but only worktree related change in the\n> submodule. The read-tree call in the submodule is not amused about\n> the duplicate argument.\n\nWhat is the symptom of read-tree being unamused? Is that a sign that\nthese patches are out of order (i.e. that we should prepare to handle an\nunchanged submodule first, and then adjust the caller to pass in\nunchanged submodules)?\n\n> It turns out that we can omit the duplicate old argument in all forced\n> cases anyway, so let's do that.\n\nWhat is the end-user visibile effect of such a change? E.g. what\nbecomes possible to a user that wasn't possible before?\n\nAmong the commands you mentioned before:\n\n checkout -f\n\tI think I would expect this not to touch a submodule that\n\thasn't changed, since that would be consistent with its\n\tbehavior on files that haven't changed.\n\n reset --hard\n\tNice! Yes, recursing into unchanged submodules is a big part\n\tof the psychological comfort of being able to say \"you can\n\talways use reset --hard to get back to a known\n\tstate\".\n\n read-tree -u --reset\n\tThis is essentially the plumbing equivalent of reset --hard,\n\tso it makes sense for them to be consistent. Ok.\n\nBased on the checkout -f case, if I've understood correctly then patch\n4/5 goes too far on it (but I could easily be convinced otherwise).\n\n> Signed-off-by: Stefan Beller \n> ---\n> submodule.c | 4 +++-\n> t/lib-submodule-update.sh | 2 +-\n> 2 files changed, 4 insertions(+), 2 deletions(-)\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\nInteresting. What is the effect when old != new?\n\nThanks,\nJonathan\n"},{"id":"335058","messageId":"CAGZ79kake8k2dM=NPwNoqB5Vkxy+k67PACz01-aXx6-njcisgQ@mail.gmail.com","threadId":"47468","inReplyTo":"20171219224431.GG240141@aiede.mtv.corp.google.com","subject":"Re: [PATCH 5/5] submodule: submodule_move_head omits old argument in forced case","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-12-19T22:54:53Z","receivedAt":"2017-12-19T22:55:00Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Tue, Dec 19, 2017 at 2:44 PM, Jonathan Nieder wrote:\n> Hi,\n>\n> I had trouble understanding what this fixes, so I'll try nitpicking a\n> bit as a sideways way to address that.\n>\n> Stefan Beller wrote:\n>\n>> With the previous patch applied (fix of the same() function),\n>\n> This tripped me up a bit. Usually commits assume that all previous\n> patches have already been applied, since that allows the maintainer to\n> apply the early part of a series on one day and the later part another\n> day without losing too much context.\n>\n> I think this intends to say something like\n>\n> Now that we allow recursing into an unchanged submodule (see\n> \"unpack-trees: Fix same() for submodules\", 2017-12-19), it is\n> possible for ...\n\n\nyup\n\n>\n>> the\n>> function `submodule_move_head` may be invoked with the same argument\n>> for the `old` and `new` state of a submodule, for example when you\n>> run `reset --hard --recurse-submodules` in the superproject that has no\n>> change in the gitlink entry, but only worktree related change in the\n>> submodule. The read-tree call in the submodule is not amused about\n>> the duplicate argument.\n>\n> What is the symptom of read-tree being unamused? Is that a sign that\n> these patches are out of order (i.e. that we should prepare to handle an\n> unchanged submodule first, and then adjust the caller to pass in\n> unchanged submodules)?\n>\n>> It turns out that we can omit the duplicate old argument in all forced\n>> cases anyway, so let's do that.\n>\n> What is the end-user visibile effect of such a change? E.g. what\n> becomes possible to a user that wasn't possible before?\n>\n> Among the commands you mentioned before:\n>\n> checkout -f\n> I think I would expect this not to touch a submodule that\n> hasn't changed, since that would be consistent with its\n> behavior on files that haven't changed.\n>\n> reset --hard\n> Nice! Yes, recursing into unchanged submodules is a big part\n> of the psychological comfort of being able to say \"you can\n> always use reset --hard to get back to a known\n> state\".\n\nI may have a different understanding of git commands than you do,\nbut a plain \"checkout -f\" with no further arguments is the same as\na hard reset IMHO, and could be used interchangeably?\n\nRehashing the \"What is a submodule?\" discussion, I would claim\nthat when its worktree is changed, we'd want checkout to restore\nthe submodules worktree back, so I disagree with your assessment\nof checkout -f.\n\n> read-tree -u --reset\n> This is essentially the plumbing equivalent of reset --hard,\n> so it makes sense for them to be consistent. Ok.\n>\n> Based on the checkout -f case, if I've understood correctly then patch\n> 4/5 goes too far on it (but I could easily be convinced otherwise).\n\nWithout 4/5 we cannot implement hard reset recursing into submodules\nas it is functionally the same as forced checkout except when we\ndifferentiate them\non a higher layer?\n\n>> Signed-off-by: Stefan Beller \n>> ---\n>> submodule.c | 4 +++-\n>> t/lib-submodule-update.sh | 2 +-\n>> 2 files changed, 4 insertions(+), 2 deletions(-)\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>> else\n>> argv_array_push(&cp.args, \"-m\");\n>>\n>> - argv_array_push(&cp.args, old ? old : EMPTY_TREE_SHA1_HEX);\n>> + if (!(flags & SUBMODULE_MOVE_HEAD_FORCE))\n>> + argv_array_push(&cp.args, old ? old : EMPTY_TREE_SHA1_HEX);\n>\n> Interesting. What is the effect when old != new?\n\nwhen the force flag is set we mostly pass in old=\"HEAD\", which is\ntechnically correct,\nbut not a sha1. (I assume you want to know what happens when two unequal sha1s\nare given; for that it will perform a 2 way merge instead of a complete reset)\n\nThanks,\nStefan\n"},{"id":"335059","messageId":"20171219231544.GH240141@aiede.mtv.corp.google.com","threadId":"47468","inReplyTo":"CAGZ79kake8k2dM=NPwNoqB5Vkxy+k67PACz01-aXx6-njcisgQ@mail.gmail.com","subject":"Re: [PATCH 5/5] submodule: submodule_move_head omits old argument in forced case","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2017-12-19T23:15:44Z","receivedAt":"2017-12-19T23:15:54Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nStefan Beller wrote:\n> On Tue, Dec 19, 2017 at 2:44 PM, Jonathan Nieder wrote:\n\n>> checkout -f\n>> I think I would expect this not to touch a submodule that\n>> hasn't changed, since that would be consistent with its\n>> behavior on files that haven't changed.\n[...]\n> I may have a different understanding of git commands than you do,\n> but a plain \"checkout -f\" with no further arguments is the same as\n> a hard reset IMHO, and could be used interchangeably?\n\nAh, good catch. Filed https://crbug.com/git/5 to clarify this in\ndocumentation. Thanks for clarifying.\n\nJonathan\n"},{"id":"335061","messageId":"20171220000148.GJ240141@aiede.mtv.corp.google.com","threadId":"47468","inReplyTo":"CAGZ79kake8k2dM=NPwNoqB5Vkxy+k67PACz01-aXx6-njcisgQ@mail.gmail.com","subject":"Re: [PATCH 5/5] submodule: submodule_move_head omits old argument in forced case","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2017-12-20T00:01:48Z","receivedAt":"2017-12-20T00:01:56Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Stefan Beller wrote:\n> On Tue, Dec 19, 2017 at 2:44 PM, Jonathan Nieder wrote:\n\n>> checkout -f\n>> I think I would expect this not to touch a submodule that\n>> hasn't changed, since that would be consistent with its\n>> behavior on files that haven't changed.\n[...]\n> I may have a different understanding of git commands than you do,\n> but a plain \"checkout -f\" with no further arguments is the same as\n> a hard reset IMHO, and could be used interchangeably?\n\nA kind person pointed out privately that you were talking about\n\"git checkout -f\" with no further arguments, not \"git checkout -f\n\". In that context, the competing meanings I mentioned in\nhttps://crbug.com/git/5 don't exist: either a given entry in the\nworktree matches the index or it doesn't.\n\nSo plain \"git checkout -f\" is similar to plain \"git reset --hard\"\nin that it means \"make the worktree (and index, in the reset case)\nlook just like this\". checkout -f makes the worktree look like the\nindex; reset --hard makes the worktree and index look like HEAD.\n\nIn that context, more aggressively making the submodule in the\nworktree get into the defined state sounds to me like a good change.\n\nHopefully my confusion helps illustrate what a commit message focusing\non the end user experience might want to mention.\n\nThanks again,\nJonathan\n"},{"id":"335098","messageId":"CAGZ79kZpY8iXhCUOB=KuwGPOr7zvV7z+Q-L43p28=7ydaTvutg@mail.gmail.com","threadId":"47468","inReplyTo":"20171220000148.GJ240141@aiede.mtv.corp.google.com","subject":"Re: [PATCH 5/5] submodule: submodule_move_head omits old argument in forced case","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-12-20T19:36:19Z","receivedAt":"2017-12-20T19:36:27Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Tue, Dec 19, 2017 at 4:01 PM, Jonathan Nieder wrote:\n> Stefan Beller wrote:\n>> On Tue, Dec 19, 2017 at 2:44 PM, Jonathan Nieder wrote:\n>\n>>> checkout -f\n>>> I think I would expect this not to touch a submodule that\n>>> hasn't changed, since that would be consistent with its\n>>> behavior on files that haven't changed.\n> [...]\n>> I may have a different understanding of git commands than you do,\n>> but a plain \"checkout -f\" with no further arguments is the same as\n>> a hard reset IMHO, and could be used interchangeably?\n>\n> A kind person pointed out privately that you were talking about\n> \"git checkout -f\" with no further arguments, not \"git checkout -f\n> \". In that context, the competing meanings I mentioned in\n> https://crbug.com/git/5 don't exist: either a given entry in the\n> worktree matches the index or it doesn't.\n>\n> So plain \"git checkout -f\" is similar to plain \"git reset --hard\"\n> in that it means \"make the worktree (and index, in the reset case)\n> look just like this\".\n\nwith \"this\" == the argument that was given, if the argument\nwas omitted, HEAD is assumed.\n\n> checkout -f makes the worktree look like the index;\n\nNo, here is what my installation of git (recent master) does:\n\n $ git --version\ngit version 2.15.1.389.g52015aaf9d\n\n $ cat test.sh\n\n rm -rf tmp\n git init tmp\n\n cd tmp\n git commit --allow-empty -m initial\n echo commit >a\n echo commit >b\n echo commit >c\n git add .\n git commit -m commit\n\n echo index >a\n git add a\n echo worktree >a\n\n echo index >b\n git add b\n\n echo worktree>c\n\n $ sh test.sh\nInitialized empty Git repository in /u/tmp/.git/\n[master (root-commit) c109fb5] initial\n[master fcc21ea] commit\n 3 files changed, 3 insertions(+)\n create mode 100644 a\n create mode 100644 b\n create mode 100644 c\n $ cd tmp\n $ git status\nOn branch master\nChanges to be committed:\n (use \"git reset HEAD ...\" to unstage)\n\nmodified: a\nmodified: b\n\nChanges not staged for commit:\n (use \"git add ...\" to update what will be committed)\n (use \"git checkout -- ...\" to discard changes in working directory)\n\nmodified: a\nmodified: c\n\n $ git checkout -f\n $ git status\nOn branch master\nnothing to commit, working tree clean\n\n# Let's test the other commands as well:\n $ cd ..\n $ sh test.sh\n ...\n $ cd tmp\n $ git checkout -f HEAD\n $ git status\nOn branch master\nnothing to commit, working tree clean\n\n # See, there is no difference between giving HEAD as an argument\n # or not! Try again with reset just for completeness:\n\n $ cd ..\n $ sh test.sh\n ...\n $ cd tmp\n $ git reset --hard\nHEAD is now at b71ae70 commit\n $ git status\nOn branch master\nnothing to commit, working tree clean\n\n # The only difference between reset and the checkout is that reset\n # tells me where we are.\n\n> reset --hard makes the worktree and index look like HEAD.\n\nI agree.\n\n> In that context, more aggressively making the submodule in the\n> worktree get into the defined state sounds to me like a good change.\n>\n> Hopefully my confusion helps illustrate what a commit message focusing\n> on the end user experience might want to mention.\n\nI'll try to come up with a better commit message. Thanks for bearing\nwith me here.\n\nStefan\n\n\n>\n> Thanks again,\n> Jonathan\n"},{"id":"335099","messageId":"CAGZ79kZ37hwHtu=n3He9P5p6mXkoRgL6yVo5Xz07yrUYKm7p7A@mail.gmail.com","threadId":"47468","inReplyTo":"CAGZ79kZpY8iXhCUOB=KuwGPOr7zvV7z+Q-L43p28=7ydaTvutg@mail.gmail.com","subject":"Re: [PATCH 5/5] submodule: submodule_move_head omits old argument in forced case","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-12-20T19:38:35Z","receivedAt":"2017-12-20T19:38:41Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Wed, Dec 20, 2017 at 11:36 AM, Stefan Beller wrote:\n> On Tue, Dec 19, 2017 at 4:01 PM, Jonathan Nieder wrote:\n>> Stefan Beller wrote:\n>>> On Tue, Dec 19, 2017 at 2:44 PM, Jonathan Nieder wrote:\n>>\n>>>> checkout -f\n>>>> I think I would expect this not to touch a submodule that\n>>>> hasn't changed, since that would be consistent with its\n>>>> behavior on files that haven't changed.\n>> [...]\n>>> I may have a different understanding of git commands than you do,\n>>> but a plain \"checkout -f\" with no further arguments is the same as\n>>> a hard reset IMHO, and could be used interchangeably?\n>>\n>> A kind person pointed out privately that you were talking about\n>> \"git checkout -f\" with no further arguments, not \"git checkout -f\n>> \". In that context, the competing meanings I mentioned in\n>> https://crbug.com/git/5 don't exist: either a given entry in the\n>> worktree matches the index or it doesn't.\n>>\n>> So plain \"git checkout -f\" is similar to plain \"git reset --hard\"\n>> in that it means \"make the worktree (and index, in the reset case)\n>> look just like this\".\n>\n> with \"this\" == the argument that was given, if the argument\n> was omitted, HEAD is assumed.\n>\n>> checkout -f makes the worktree look like the index;\n>\n> No, here is what my installation of git (recent master) does:\n\nWell, you are technically correct, the worktree is made look like the index,\nbut prior to that the index is reset to the HEAD, so for me it is\neasier to understand\nas \"make worktree and index like HEAD\"\n"},{"id":"335183","messageId":"xmqq7etesgju.fsf@gitster.mtv.corp.google.com","threadId":"47468","inReplyTo":"20171219222636.216001-3-sbeller@google.com","subject":"Re: [PATCH 2/5] t/lib-submodule-update.sh: Fix test ignoring ignored files in submodules","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-12-22T19:34:13Z","receivedAt":"2017-12-22T19:34:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> It turns out that this buggy test passed due to the buggy implementation,\n> which will soon be corrected. Let's fix the test first.\n\nPlease clarify \"buggy test\". Without the original discussion (I am\nimagining there is something that happened on the list recently),\nhere is my guess:\n\n We tried to make sure that an ignored file is $distimmed by\n $gostak, but forgot to tell the exclude mechanism that the path\n 'sub1'ignored' we use for the test is actually ignored, so the\n fact that the file was not $distimmed when $gostak did its thing\n meant nothing---mark any path whose name is 'ignored' as ignored\n to really test the condition we want to test.\n\nbut I do not exactly know what verb to replace $distim and what noun\nto replace $gostak above ;-)\n\nAlso, wouldn't this expose a bug in the implementation if that is\nthe case?\n\nThanks.\n\n>\n> Signed-off-by: Stefan Beller \n> ---\n> t/lib-submodule-update.sh | 1 +\n> 1 file changed, 1 insertion(+)\n>\n> diff --git a/t/lib-submodule-update.sh b/t/lib-submodule-update.sh\n> index 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"},{"id":"335389","messageId":"CAGZ79kaPYsR2PBVz-XN3u0eDgrdh=tuqdMhqTkS0VnrDkn7dTA@mail.gmail.com","threadId":"47468","inReplyTo":"xmqq7etesgju.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 2/5] t/lib-submodule-update.sh: Fix test ignoring ignored files in submodules","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-12-27T19:27:50Z","receivedAt":"2017-12-27T19:27:57Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Fri, Dec 22, 2017 at 11:34 AM, Junio C Hamano wrote:\n> Stefan Beller writes:\n>\n>> It turns out that this buggy test passed due to the buggy implementation,\n>> which will soon be corrected. Let's fix the test first.\n>\n> Please clarify \"buggy test\". Without the original discussion (I am\n> imagining there is something that happened on the list recently),\n> here is my guess:\n>\n> We tried to make sure that an ignored file is $distimmed by\n> $gostak, but forgot to tell the exclude mechanism that the path\n> 'sub1'ignored' we use for the test is actually ignored, so the\n> fact that the file was not $distimmed when $gostak did its thing\n> meant nothing---mark any path whose name is 'ignored' as ignored\n> to really test the condition we want to test.\n>\n> but I do not exactly know what verb to replace $distim and what noun\n> to replace $gostak above ;-)\n\nJust rewrote that commit message without distims and gostaking.\n\n> Also, wouldn't this expose a bug in the implementation if that is\n> the case?\n\nYes it is, which will be fixed. The bug is:\n\nIf the submodule HEAD is at the superprojects recorded object\nname, we don't bother looking into the submodule at all, neither for\ndeleting untracked files, nor resetting changed tracked files.\n\nThanks,\n\nStefan\n"},{"id":"335400","messageId":"20171227225705.73235-1-sbeller@google.com","threadId":"47468","inReplyTo":"20171219222636.216001-1-sbeller@google.com","subject":"[PATCHv2 0/5] Fix --recurse-submodules for submodule worktree changes","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-12-27T22:57:00Z","receivedAt":"2017-12-27T22:57:17Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"I dropped the patch to `same()` as I realized we only need to fix the\noneway_merge function, the others (two, three way merge) are fine as\nthey have the checks already in place.\n\nThe test added in the last patch got slightly larger as now we also test for\nnewly staged files to be blown away in the submodule.\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\nStefan Beller (5):\n t/helper/test-lazy-name-hash: fix compilation\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: oneway_merge to update submodules\n submodule: submodule_move_head omits old argument in forced case\n\n submodule.c | 4 +++-\n t/helper/test-lazy-init-name-hash.c | 2 +-\n t/lib-submodule-update.sh | 19 +++++++++++++++++--\n unpack-trees.c | 3 +++\n 4 files changed, 24 insertions(+), 4 deletions(-)\n\n-- \n2.15.1.620.gb9897f4670-goog\n\n"},{"id":"335401","messageId":"20171227225705.73235-2-sbeller@google.com","threadId":"47468","inReplyTo":"20171227225705.73235-1-sbeller@google.com","subject":"[PATCHv2 1/5] t/helper/test-lazy-name-hash: fix compilation","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-12-27T22:57:01Z","receivedAt":"2017-12-27T22:57:20Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"I was compiling origin/master today with stricter compiler flags today\nand was greeted by\n\nt/helper/test-lazy-init-name-hash.c: In function ‘cmd_main’:\nt/helper/test-lazy-init-name-hash.c:172:5: error: ‘nr_threads_used’ may be used uninitialized in this function [-Werror=maybe-uninitialized]\n printf(\"avg [size %8d] [single %f] %c [multi %f %d]\\n\",\n ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~\n nr,\n ~~~\n (double)avg_single/1000000000,\n ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~\n (avg_single < avg_multi ? '<' : '>'),\n ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~\n (double)avg_multi/1000000000,\n ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~\n nr_threads_used);\n ~~~~~~~~~~~~~~~~\nt/helper/test-lazy-init-name-hash.c:115:6: note: ‘nr_threads_used’ was declared here\n int nr_threads_used;\n ^~~~~~~~~~~~~~~\n\nI do not see how we can arrive at that line without having `nr_threads_used`\ninitialized, as we'd have `count > 1` (which asserts that we ran the\nloop above at least once, such that it *should* be initialized).\n\nI do not have time to dive into further analysis.\n\nSigned-off-by: Stefan Beller \n---\n t/helper/test-lazy-init-name-hash.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/t/helper/test-lazy-init-name-hash.c b/t/helper/test-lazy-init-name-hash.c\nindex 6368a89345..297fb01d61 100644\n--- a/t/helper/test-lazy-init-name-hash.c\n+++ b/t/helper/test-lazy-init-name-hash.c\n@@ -112,7 +112,7 @@ static void analyze_run(void)\n {\n \tuint64_t t1s, t1m, t2s, t2m;\n \tint cache_nr_limit;\n-\tint nr_threads_used;\n+\tint nr_threads_used = 0;\n \tint i;\n \tint nr;\n \n-- \n2.15.1.620.gb9897f4670-goog\n\n"},{"id":"335402","messageId":"20171227225705.73235-3-sbeller@google.com","threadId":"47468","inReplyTo":"20171227225705.73235-1-sbeller@google.com","subject":"[PATCHv2 2/5] t/lib-submodule-update.sh: clarify test","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-12-27T22:57:02Z","receivedAt":"2017-12-27T22:57:22Z","isPatch":false,"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 \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":"335403","messageId":"20171227225705.73235-4-sbeller@google.com","threadId":"47468","inReplyTo":"20171227225705.73235-1-sbeller@google.com","subject":"[PATCHv2 3/5] t/lib-submodule-update.sh: Fix test ignoring ignored files in submodules","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-12-27T22:57:03Z","receivedAt":"2017-12-27T22:57:24Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"It turns out that the test replacing a submodule with a file with\nthe submodule containing an ignored file is incorrectly titled,\nbecause the test put the file in place, but never ignored that file.\nWhen having an untracked file Instead of an ignored file in the\nsubmodule, git should refuse to remove the submodule, but that is\na bug in the implementation of recursing into submodules, such that\nthe test just passed, removing the untracked file.\n\nFix the test first; in a later patch we'll fix gits behavior,\nthat will make sure untracked files are not deleted.\n\nSigned-off-by: Stefan Beller \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":"335404","messageId":"20171227225705.73235-6-sbeller@google.com","threadId":"47468","inReplyTo":"20171227225705.73235-1-sbeller@google.com","subject":"[PATCHv2 5/5] submodule: submodule_move_head omits old argument in forced case","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-12-27T22:57:05Z","receivedAt":"2017-12-27T22:57:27Z","isPatch":false,"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 \n---\n submodule.c | 4 +++-\n t/lib-submodule-update.sh | 14 ++++++++++++++\n 2 files changed, 17 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..1f38a85371 100755\n--- a/t/lib-submodule-update.sh\n+++ b/t/lib-submodule-update.sh\n@@ -1015,4 +1015,18 @@ 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: >sub1/new_file &&\n+\t\t\tgit -C sub1 add new_file &&\n+\t\t\t$command HEAD &&\n+\t\t\ttest_path_is_file sub1/file1 &&\n+\t\t\ttest_path_is_missing sub1/new_file\n+\t\t)\n+\t'\n }\n-- \n2.15.1.620.gb9897f4670-goog\n\n"},{"id":"335405","messageId":"20171227225705.73235-5-sbeller@google.com","threadId":"47468","inReplyTo":"20171227225705.73235-1-sbeller@google.com","subject":"[PATCHv2 4/5] unpack-trees: oneway_merge to update submodules","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-12-27T22:57:04Z","receivedAt":"2017-12-27T22:57:30Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"When there is a one way merge, each submodule needs to be one way merged\nas well, if we're asked to recurse into submodules.\n\nIn case of a submodule, check if it is up-to-date, otherwise set the\nflag CE_UPDATE, which will trigger an update of it in the phase updating\nthe tree later.\n\nSigned-off-by: Stefan Beller \n---\n unpack-trees.c | 3 +++\n 1 file changed, 3 insertions(+)\n\ndiff --git a/unpack-trees.c b/unpack-trees.c\nindex bf8b602901..ac59524a7f 100644\n--- a/unpack-trees.c\n+++ b/unpack-trees.c\n@@ -2139,6 +2139,9 @@ int oneway_merge(const struct cache_entry * const *src,\n \t\t\t ie_match_stat(o->src_index, old, &st, CE_MATCH_IGNORE_VALID|CE_MATCH_IGNORE_SKIP_WORKTREE))\n \t\t\t\tupdate |= CE_UPDATE;\n \t\t}\n+\t\tif (S_ISGITLINK(old->ce_mode) && should_update_submodules() &&\n+\t\t !verify_uptodate(old, o))\n+\t\t\tupdate |= CE_UPDATE;\n \t\tadd_entry(o, old, update, 0);\n \t\treturn 0;\n \t}\n-- \n2.15.1.620.gb9897f4670-goog\n\n"},{"id":"335624","messageId":"xmqqbmichws2.fsf@gitster.mtv.corp.google.com","threadId":"47468","inReplyTo":"20171227225705.73235-5-sbeller@google.com","subject":"Re: [PATCHv2 4/5] unpack-trees: oneway_merge to update submodules","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-01-02T19:43:09Z","receivedAt":"2018-01-02T19:43:17Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> diff --git a/unpack-trees.c b/unpack-trees.c\n> index bf8b602901..ac59524a7f 100644\n> --- a/unpack-trees.c\n> +++ b/unpack-trees.c\n> @@ -2139,6 +2139,9 @@ int oneway_merge(const struct cache_entry * const *src,\n> \t\t\t ie_match_stat(o->src_index, old, &st, CE_MATCH_IGNORE_VALID|CE_MATCH_IGNORE_SKIP_WORKTREE))\n> \t\t\t\tupdate |= CE_UPDATE;\n> \t\t}\n> +\t\tif (S_ISGITLINK(old->ce_mode) && should_update_submodules() &&\n> +\t\t !verify_uptodate(old, o))\n> +\t\t\tupdate |= CE_UPDATE;\n> \t\tadd_entry(o, old, update, 0);\n> \t\treturn 0;\n> \t}\n\nThis is more sensible than the previous one that broke same() by\nunconditionally checking the working tree state, even when the\nmachinery is told not to look at the working tree.\n\nThe code in the pre-context of this hunk, (i.e. the original one you\nare half-way mimicking), we realize that the original cache entry\nand the cache entry we are checking out are exactly the same and we\noverwrite when we know that the path in the working tree is dirty,\nand we are not told to \"skip\" that path in the working tree\n(i.e. sparse checkout). That happens only when we are told to\no->update and o->reset.\n\nShouldn't we be setting the update bit only when o->update is in\neffect, just like we can see in the pre-context of this hunk? I do\nnot know if we need to require o->reset and/or need to pay attention\nto the skip worktree bit in this new case.\n"},{"id":"335641","messageId":"CAGZ79kaogOLj4e38aWfzOr9TwMGngDjCL9+weFm2Jm4gTtSjEA@mail.gmail.com","threadId":"47468","inReplyTo":"xmqqbmichws2.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCHv2 4/5] unpack-trees: oneway_merge to update submodules","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-01-02T23:04:16Z","receivedAt":"2018-01-02T23:04:23Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Tue, Jan 2, 2018 at 11:43 AM, Junio C Hamano wrote:\n> Stefan Beller writes:\n>\n>> diff --git a/unpack-trees.c b/unpack-trees.c\n>> index bf8b602901..ac59524a7f 100644\n>> --- a/unpack-trees.c\n>> +++ b/unpack-trees.c\n>> @@ -2139,6 +2139,9 @@ int oneway_merge(const struct cache_entry * const *src,\n>> ie_match_stat(o->src_index, old, &st, CE_MATCH_IGNORE_VALID|CE_MATCH_IGNORE_SKIP_WORKTREE))\n>> update |= CE_UPDATE;\n>> }\n>> + if (S_ISGITLINK(old->ce_mode) && should_update_submodules() &&\n>> + !verify_uptodate(old, o))\n>> + update |= CE_UPDATE;\n>> add_entry(o, old, update, 0);\n>> return 0;\n>> }\n>\n> This is more sensible than the previous one that broke same() by\n> unconditionally checking the working tree state, even when the\n> machinery is told not to look at the working tree.\n>\n> The code in the pre-context of this hunk, (i.e. the original one you\n> are half-way mimicking), we realize that the original cache entry\n> and the cache entry we are checking out are exactly the same and we\n> overwrite when we know that the path in the working tree is dirty,\n> and we are not told to \"skip\" that path in the working tree\n> (i.e. sparse checkout). That happens only when we are told to\n> o->update and o->reset.\n>\n> Shouldn't we be setting the update bit only when o->update is in\n> effect,\n\nYes, we should.\n\n> just like we can see in the pre-context of this hunk? I do\n> not know if we need to require o->reset and/or need to pay attention\n> to the skip worktree bit in this new case.\n\nverify_uptodate takes care of the worktree bits\n(\"o->skip_sparse_checkout && (ce->ce_flags & CE_NEW_SKIP_WORKTREE))\")\n\no->reset is taken care of inside the nested verify_uptodate_1 function via\n\n ...\n else if (o->reset || ce_uptodate(ce))\n return 0;\n\nSo I'd think we only need to toss in the o->update check.\n\nAnd that additional check would only be a performance optimization\nas it would omit the check in case we do not want to update.\n(verify_uptodate is expensive for submodules)\n"},{"id":"335683","messageId":"20180103011226.160185-1-sbeller@google.com","threadId":"47468","inReplyTo":"xmqqbmichws2.fsf@gitster.mtv.corp.google.com","subject":"[PATCHv3 0/5] Fix --recurse-submodules for submodule worktree changes","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-01-03T01:12:21Z","receivedAt":"2018-01-03T01:12:42Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Thanks Junio for review of this series!\nThe only change in this version of the series is\n\n--- a/unpack-trees.c\n+++ b/unpack-trees.c\n@@ -2140,7 +2140,7 @@ int oneway_merge(const struct cache_entry * const *src,\n update |= CE_UPDATE;\n }\n if (S_ISGITLINK(old->ce_mode) && should_update_submodules() &&\n- !verify_uptodate(old, o))\n+ o->update && !verify_uptodate(old, o))\n update |= CE_UPDATE;\n add_entry(o, old, update, 0);\n\n\nv2:\nI dropped the patch to `same()` as I realized we only need to fix the\noneway_merge function, the others (two, three way merge) are fine as\nthey have the checks already in place.\n\nThe test added in the last patch got slightly larger as now we also test for\nnewly staged files to be blown away in the submodule.\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\nStefan Beller (5):\n t/helper/test-lazy-name-hash: fix compilation\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: oneway_merge to update submodules\n submodule: submodule_move_head omits old argument in forced case\n\n submodule.c | 4 +++-\n t/helper/test-lazy-init-name-hash.c | 2 +-\n t/lib-submodule-update.sh | 19 +++++++++++++++++--\n unpack-trees.c | 3 +++\n 4 files changed, 24 insertions(+), 4 deletions(-)\n\n-- \n2.15.1.620.gb9897f4670-goog\n\n"},{"id":"335684","messageId":"20180103011226.160185-2-sbeller@google.com","threadId":"47468","inReplyTo":"20180103011226.160185-1-sbeller@google.com","subject":"[PATCH 1/5] t/helper/test-lazy-name-hash: fix compilation","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-01-03T01:12:22Z","receivedAt":"2018-01-03T01:12:44Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"I was compiling origin/master today with stricter compiler flags today\nand was greeted by\n\nt/helper/test-lazy-init-name-hash.c: In function ‘cmd_main’:\nt/helper/test-lazy-init-name-hash.c:172:5: error: ‘nr_threads_used’ may be used uninitialized in this function [-Werror=maybe-uninitialized]\n printf(\"avg [size %8d] [single %f] %c [multi %f %d]\\n\",\n ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~\n nr,\n ~~~\n (double)avg_single/1000000000,\n ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~\n (avg_single < avg_multi ? '<' : '>'),\n ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~\n (double)avg_multi/1000000000,\n ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~\n nr_threads_used);\n ~~~~~~~~~~~~~~~~\nt/helper/test-lazy-init-name-hash.c:115:6: note: ‘nr_threads_used’ was declared here\n int nr_threads_used;\n ^~~~~~~~~~~~~~~\n\nI do not see how we can arrive at that line without having `nr_threads_used`\ninitialized, as we'd have `count > 1` (which asserts that we ran the\nloop above at least once, such that it *should* be initialized).\n\nI do not have time to dive into further analysis.\n\nSigned-off-by: Stefan Beller \n---\n t/helper/test-lazy-init-name-hash.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/t/helper/test-lazy-init-name-hash.c b/t/helper/test-lazy-init-name-hash.c\nindex 6368a89345..297fb01d61 100644\n--- a/t/helper/test-lazy-init-name-hash.c\n+++ b/t/helper/test-lazy-init-name-hash.c\n@@ -112,7 +112,7 @@ static void analyze_run(void)\n {\n \tuint64_t t1s, t1m, t2s, t2m;\n \tint cache_nr_limit;\n-\tint nr_threads_used;\n+\tint nr_threads_used = 0;\n \tint i;\n \tint nr;\n \n-- \n2.15.1.620.gb9897f4670-goog\n\n"},{"id":"335685","messageId":"20180103011226.160185-3-sbeller@google.com","threadId":"47468","inReplyTo":"20180103011226.160185-1-sbeller@google.com","subject":"[PATCH 2/5] t/lib-submodule-update.sh: clarify test","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-01-03T01:12:23Z","receivedAt":"2018-01-03T01:12:46Z","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 \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":"335686","messageId":"20180103011226.160185-4-sbeller@google.com","threadId":"47468","inReplyTo":"20180103011226.160185-1-sbeller@google.com","subject":"[PATCH 3/5] t/lib-submodule-update.sh: Fix test ignoring ignored files in submodules","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-01-03T01:12:24Z","receivedAt":"2018-01-03T01:12:47Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"It turns out that the test replacing a submodule with a file with\nthe submodule containing an ignored file is incorrectly titled,\nbecause the test put the file in place, but never ignored that file.\nWhen having an untracked file Instead of an ignored file in the\nsubmodule, git should refuse to remove the submodule, but that is\na bug in the implementation of recursing into submodules, such that\nthe test just passed, removing the untracked file.\n\nFix the test first; in a later patch we'll fix gits behavior,\nthat will make sure untracked files are not deleted.\n\nSigned-off-by: Stefan Beller \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":"335687","messageId":"20180103011226.160185-6-sbeller@google.com","threadId":"47468","inReplyTo":"20180103011226.160185-1-sbeller@google.com","subject":"[PATCH 5/5] submodule: submodule_move_head omits old argument in forced case","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-01-03T01:12:26Z","receivedAt":"2018-01-03T01:12:50Z","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 \n---\n submodule.c | 4 +++-\n t/lib-submodule-update.sh | 14 ++++++++++++++\n 2 files changed, 17 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..1f38a85371 100755\n--- a/t/lib-submodule-update.sh\n+++ b/t/lib-submodule-update.sh\n@@ -1015,4 +1015,18 @@ 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: >sub1/new_file &&\n+\t\t\tgit -C sub1 add new_file &&\n+\t\t\t$command HEAD &&\n+\t\t\ttest_path_is_file sub1/file1 &&\n+\t\t\ttest_path_is_missing sub1/new_file\n+\t\t)\n+\t'\n }\n-- \n2.15.1.620.gb9897f4670-goog\n\n"},{"id":"335688","messageId":"20180103011226.160185-5-sbeller@google.com","threadId":"47468","inReplyTo":"20180103011226.160185-1-sbeller@google.com","subject":"[PATCH 4/5] unpack-trees: oneway_merge to update submodules","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-01-03T01:12:25Z","receivedAt":"2018-01-03T01:12:54Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"When there is a one way merge, each submodule needs to be one way merged\nas well, if we're asked to recurse into submodules.\n\nIn case of a submodule, check if it is up-to-date, otherwise set the\nflag CE_UPDATE, which will trigger an update of it in the phase updating\nthe tree later.\n\nSigned-off-by: Stefan Beller \n---\n unpack-trees.c | 3 +++\n 1 file changed, 3 insertions(+)\n\ndiff --git a/unpack-trees.c b/unpack-trees.c\nindex bf8b602901..7657d6ecdd 100644\n--- a/unpack-trees.c\n+++ b/unpack-trees.c\n@@ -2139,6 +2139,9 @@ int oneway_merge(const struct cache_entry * const *src,\n \t\t\t ie_match_stat(o->src_index, old, &st, CE_MATCH_IGNORE_VALID|CE_MATCH_IGNORE_SKIP_WORKTREE))\n \t\t\t\tupdate |= CE_UPDATE;\n \t\t}\n+\t\tif (S_ISGITLINK(old->ce_mode) && should_update_submodules() &&\n+\t\t o->update && !verify_uptodate(old, o))\n+\t\t\tupdate |= CE_UPDATE;\n \t\tadd_entry(o, old, update, 0);\n \t\treturn 0;\n \t}\n-- \n2.15.1.620.gb9897f4670-goog\n\n"},{"id":"335782","messageId":"xmqqo9mahdll.fsf@gitster.mtv.corp.google.com","threadId":"47468","inReplyTo":"20180103011226.160185-1-sbeller@google.com","subject":"Re: [PATCHv3 0/5] Fix --recurse-submodules for submodule worktree changes","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-01-03T20:49:42Z","receivedAt":"2018-01-03T20:50:05Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> Thanks Junio for review of this series!\n> The only change in this version of the series is\n>\n> --- a/unpack-trees.c\n> +++ b/unpack-trees.c\n> @@ -2140,7 +2140,7 @@ int oneway_merge(const struct cache_entry * const *src,\n> update |= CE_UPDATE;\n> }\n> if (S_ISGITLINK(old->ce_mode) && should_update_submodules() &&\n> - !verify_uptodate(old, o))\n> + o->update && !verify_uptodate(old, o))\n> update |= CE_UPDATE;\n> add_entry(o, old, update, 0);\n>\n\nSounds OK. \n\nI wonder why o->update is not at the very beginning of the &&-chain,\nthough. After all, the one above this addition begins with o->reset\n&& o->update *not* because of the performance concern, but primarily\ndue to logic flow. I.e. \"if we are resetting and updating the\nworking tree, then...\" comes first before saying \"we may need to\nflip CE_UPDATE bit in update variable if the file in the working\ntree is not up to date and it is within a narrow checkout area\".\n\nOf course, because verify_uptodate() is rather expensive, checking\no->update before that makes sense from micro-optimization's point of\nview, too.\n\nSo after thinking aloud like the above, I am reasonably sure that\nyou want to check o->update as the very first thing in this new if\nstatement.\n\n> v2:\n> I dropped the patch to `same()` as I realized we only need to fix the\n> oneway_merge function, the others (two, three way merge) are fine as\n> they have the checks already in place.\n\nThis is a bit flawed argument, no? Checking working tree paths\nunconditionally in same(), which does not even know if we are\ntouching the working tree paths, is broken. Unless \"they have the\nchecks already in place\" refers to checks that bypasses calls to\nsame() when we are not touching working tree paths, that is, but\nobviously that is not what is going on.\n\nWill queue. Thanks for working on this.\n\n\n"},{"id":"335786","messageId":"CAGZ79kbC5tsL8R-tUBxHoZzxYiqiWwYS61F7cLfTZeb0Wu1LGA@mail.gmail.com","threadId":"47468","inReplyTo":"xmqqo9mahdll.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCHv3 0/5] Fix --recurse-submodules for submodule worktree changes","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-01-03T21:16:20Z","receivedAt":"2018-01-03T21:16:26Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Wed, Jan 3, 2018 at 12:49 PM, Junio C Hamano wrote:\n> Stefan Beller writes:\n>\n>> Thanks Junio for review of this series!\n>> The only change in this version of the series is\n>>\n>> --- a/unpack-trees.c\n>> +++ b/unpack-trees.c\n>> @@ -2140,7 +2140,7 @@ int oneway_merge(const struct cache_entry * const *src,\n>> update |= CE_UPDATE;\n>> }\n>> if (S_ISGITLINK(old->ce_mode) && should_update_submodules() &&\n>> - !verify_uptodate(old, o))\n>> + o->update && !verify_uptodate(old, o))\n>> update |= CE_UPDATE;\n>> add_entry(o, old, update, 0);\n>>\n>\n> Sounds OK.\n>\n> I wonder why o->update is not at the very beginning of the &&-chain,\n> though. After all, the one above this addition begins with o->reset\n> && o->update *not* because of the performance concern, but primarily\n> due to logic flow. I.e. \"if we are resetting and updating the\n> working tree, then...\" comes first before saying \"we may need to\n> flip CE_UPDATE bit in update variable if the file in the working\n> tree is not up to date and it is within a narrow checkout area\".\n\nIt shows that I work too much with submodules. ;)\n\"If we have a submodule and ...\" seemed to be the important\npart when writing the patch.\n\n> Of course, because verify_uptodate() is rather expensive, checking\n> o->update before that makes sense from micro-optimization's point of\n> view, too.\n\nI would think S_ISGITLINK, should_update_submodules as well\nas o->update are all on the same order of magnitude of costs\n(some couple number of operations) when\ncompared to verify_uptodate (spawning processes),\nso as long as verify_uptodate goes last we'd be fine.\n\n>\n> So after thinking aloud like the above, I am reasonably sure that\n> you want to check o->update as the very first thing in this new if\n> statement.\n\nThanks for double checking and thinking about the code base with\na less submodule centric point of view.\n\nMind to squash it locally or want me to resend?\nFor a resend I'll wait a couple of days to see if there are more\ncomments needing to be addressed.\n\n\n>\n>> v2:\n>> I dropped the patch to `same()` as I realized we only need to fix the\n>> oneway_merge function, the others (two, three way merge) are fine as\n>> they have the checks already in place.\n>\n> This is a bit flawed argument, no? Checking working tree paths\n> unconditionally in same(), which does not even know if we are\n> touching the working tree paths, is broken. Unless \"they have the\n> checks already in place\" refers to checks that bypasses calls to\n> same() when we are not touching working tree paths, that is, but\n> obviously that is not what is going on.\n>\n> Will queue. Thanks for working on this.\n>\n>\n"},{"id":"335953","messageId":"20180105200304.34173-1-sbeller@google.com","threadId":"47468","inReplyTo":"xmqqo9mahdll.fsf@gitster.mtv.corp.google.com","subject":"[PATCHv4 0/4] Fix --recurse-submodules for submodule worktree changes","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-01-05T20:03:00Z","receivedAt":"2018-01-05T20:03:14Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"v4:\n* dropped the patch \"t/helper/test-lazy-name-hash: fix compilation\"\n (that snuck into v3 by accident, it is already present at\n remotes/origin/jh/memihash-opt, but I am carrying it locally in\n most branches currently.)\n* Junio pointed out the micro-optimisation below, both in terms of readability\n as well as performance\n\ndiff --git a/unpack-trees.c b/unpack-trees.c\nindex 7657d6ecdd..96c3327f19 100644\n--- a/unpack-trees.c\n+++ b/unpack-trees.c\n@@ -2139,8 +2139,8 @@ int oneway_merge(const struct cache_entry * const *src,\n \t\t\t ie_match_stat(o->src_index, old, &st, CE_MATCH_IGNORE_VALID|CE_MATCH_IGNORE_SKIP_WORKTREE))\n \t\t\t\tupdate |= CE_UPDATE;\n \t\t}\n-\t\tif (S_ISGITLINK(old->ce_mode) && should_update_submodules() &&\n-\t\t o->update && !verify_uptodate(old, o))\n+\t\tif (o->update && S_ISGITLINK(old->ce_mode) &&\n+\t\t should_update_submodules() && !verify_uptodate(old, o))\n \t\t\tupdate |= CE_UPDATE;\n \t\tadd_entry(o, old, update, 0);\n \t\treturn 0;\n\nv3:\nThanks Junio for review of this series!\nThe only change in this version of the series is\n\n--- a/unpack-trees.c\n+++ b/unpack-trees.c\n@@ -2140,7 +2140,7 @@ int oneway_merge(const struct cache_entry * const *src,\n update |= CE_UPDATE;\n }\n if (S_ISGITLINK(old->ce_mode) && should_update_submodules() &&\n- !verify_uptodate(old, o))\n+ o->update && !verify_uptodate(old, o))\n update |= CE_UPDATE;\n add_entry(o, old, update, 0);\n\n\nv2:\nI dropped the patch to `same()` as I realized we only need to fix the\noneway_merge function, the others (two, three way merge) are fine as\nthey have the checks already in place.\n\nThe test added in the last patch got slightly larger as now we also test for\nnewly staged files to be blown away in the submodule.\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\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: oneway_merge to update submodules\n submodule: submodule_move_head omits old argument in forced case\n\n submodule.c | 4 +++-\n t/lib-submodule-update.sh | 19 +++++++++++++++++--\n unpack-trees.c | 3 +++\n 3 files changed, 23 insertions(+), 3 deletions(-)\n\n-- \n2.16.0.rc0.223.g4a4ac83678-goog\n\n"},{"id":"335954","messageId":"20180105200304.34173-2-sbeller@google.com","threadId":"47468","inReplyTo":"20180105200304.34173-1-sbeller@google.com","subject":"[PATCHv4 1/4] t/lib-submodule-update.sh: clarify test","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-01-05T20:03:01Z","receivedAt":"2018-01-05T20:03:16Z","isPatch":false,"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 \nSigned-off-by: Junio C Hamano \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.16.0.rc0.223.g4a4ac83678-goog\n\n"},{"id":"335955","messageId":"20180105200304.34173-3-sbeller@google.com","threadId":"47468","inReplyTo":"20180105200304.34173-1-sbeller@google.com","subject":"[PATCHv4 2/4] t/lib-submodule-update.sh: Fix test ignoring ignored files in submodules","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-01-05T20:03:02Z","receivedAt":"2018-01-05T20:03:19Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"It turns out that the test replacing a submodule with a file with\nthe submodule containing an ignored file is incorrectly titled,\nbecause the test put the file in place, but never ignored that file.\nWhen having an untracked file Instead of an ignored file in the\nsubmodule, git should refuse to remove the submodule, but that is\na bug in the implementation of recursing into submodules, such that\nthe test just passed, removing the untracked file.\n\nFix the test first; in a later patch we'll fix gits behavior,\nthat will make sure untracked files are not deleted.\n\nSigned-off-by: Stefan Beller \nSigned-off-by: Junio C Hamano \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.16.0.rc0.223.g4a4ac83678-goog\n\n"},{"id":"335956","messageId":"20180105200304.34173-4-sbeller@google.com","threadId":"47468","inReplyTo":"20180105200304.34173-1-sbeller@google.com","subject":"[PATCHv4 3/4] unpack-trees: oneway_merge to update submodules","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-01-05T20:03:03Z","receivedAt":"2018-01-05T20:03:23Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"When there is a one way merge, each submodule needs to be one way merged\nas well, if we're asked to recurse into submodules.\n\nIn case of a submodule, check if it is up-to-date, otherwise set the\nflag CE_UPDATE, which will trigger an update of it in the phase updating\nthe tree later.\n\nSigned-off-by: Stefan Beller \nSigned-off-by: Junio C Hamano \n---\n unpack-trees.c | 3 +++\n 1 file changed, 3 insertions(+)\n\ndiff --git a/unpack-trees.c b/unpack-trees.c\nindex bf8b602901..96c3327f19 100644\n--- a/unpack-trees.c\n+++ b/unpack-trees.c\n@@ -2139,6 +2139,9 @@ int oneway_merge(const struct cache_entry * const *src,\n \t\t\t ie_match_stat(o->src_index, old, &st, CE_MATCH_IGNORE_VALID|CE_MATCH_IGNORE_SKIP_WORKTREE))\n \t\t\t\tupdate |= CE_UPDATE;\n \t\t}\n+\t\tif (o->update && S_ISGITLINK(old->ce_mode) &&\n+\t\t should_update_submodules() && !verify_uptodate(old, o))\n+\t\t\tupdate |= CE_UPDATE;\n \t\tadd_entry(o, old, update, 0);\n \t\treturn 0;\n \t}\n-- \n2.16.0.rc0.223.g4a4ac83678-goog\n\n"},{"id":"335957","messageId":"20180105200304.34173-5-sbeller@google.com","threadId":"47468","inReplyTo":"20180105200304.34173-1-sbeller@google.com","subject":"[PATCHv4 4/4] submodule: submodule_move_head omits old argument in forced case","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-01-05T20:03:04Z","receivedAt":"2018-01-05T20:03:25Z","isPatch":false,"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 \nSigned-off-by: Junio C Hamano \n---\n submodule.c | 4 +++-\n t/lib-submodule-update.sh | 14 ++++++++++++++\n 2 files changed, 17 insertions(+), 1 deletion(-)\n\ndiff --git a/submodule.c b/submodule.c\nindex 2967704317..47ddc9b273 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -1657,7 +1657,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..1f38a85371 100755\n--- a/t/lib-submodule-update.sh\n+++ b/t/lib-submodule-update.sh\n@@ -1015,4 +1015,18 @@ 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: >sub1/new_file &&\n+\t\t\tgit -C sub1 add new_file &&\n+\t\t\t$command HEAD &&\n+\t\t\ttest_path_is_file sub1/file1 &&\n+\t\t\ttest_path_is_missing sub1/new_file\n+\t\t)\n+\t'\n }\n-- \n2.16.0.rc0.223.g4a4ac83678-goog\n\n"},{"id":"336327","messageId":"xmqqlgh63b44.fsf@gitster.mtv.corp.google.com","threadId":"47468","inReplyTo":"20180105200304.34173-1-sbeller@google.com","subject":"Re: [PATCHv4 0/4] Fix --recurse-submodules for submodule worktree changes","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-01-09T22:45:15Z","receivedAt":"2018-01-09T22:45:22Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> v4:\n> Stefan 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: oneway_merge to update submodules\n> submodule: submodule_move_head omits old argument in forced case\n>\n> submodule.c | 4 +++-\n> t/lib-submodule-update.sh | 19 +++++++++++++++++--\n> unpack-trees.c | 3 +++\n> 3 files changed, 23 insertions(+), 3 deletions(-)\n\nThanks. This one looks excellent.\n\nLet's merge it to 'next' and cook it there.\n"}]}