* [PATCH 0/5] Fix --recurse-submodules for submodule worktree changes @ 2017-12-19 22:26 Stefan Beller 2017-12-19 22:26 ` [PATCH 1/5] t/lib-submodule-update.sh: clarify test Stefan Beller ` (5 more replies) 0 siblings, 6 replies; 38+ messages in thread From: Stefan Beller @ 2017-12-19 22:26 UTC (permalink / raw) To: git; +Cc: jrnieder, Stefan Beller The fix is in the last patch, the first patches are just massaging the code base to make the fix easy. The second patch fixes a bug in the test, which was ineffective at testing. The third patch shows the problem this series addresses, the fourth patch is a little refactoring, which I want to keep separate as I would expect it to be a performance regression[1]. The first patch is unrelated, but improves the readability of submodule test cases, which we'd want to improve further. Thanks, Stefan [1] The performance should improve once we don't use so many processes any more, but that repository series is stalled for now. Stefan Beller (5): t/lib-submodule-update.sh: clarify test t/lib-submodule-update.sh: Fix test ignoring ignored files in submodules t/lib-submodule-update.sh: add new test for submodule internal change unpack-trees: Fix same() for submodules submodule: submodule_move_head omits old argument in forced case submodule.c | 4 +++- t/lib-submodule-update.sh | 16 ++++++++++++++-- unpack-trees.c | 2 ++ 3 files changed, 19 insertions(+), 3 deletions(-) -- 2.15.1.620.gb9897f4670-goog ^ permalink raw reply [flat|nested] 38+ messages in thread
* [PATCH 1/5] t/lib-submodule-update.sh: clarify test 2017-12-19 22:26 [PATCH 0/5] Fix --recurse-submodules for submodule worktree changes Stefan Beller @ 2017-12-19 22:26 ` Stefan Beller 2017-12-19 22:26 ` [PATCH 2/5] t/lib-submodule-update.sh: Fix test ignoring ignored files in submodules Stefan Beller ` (4 subsequent siblings) 5 siblings, 0 replies; 38+ messages in thread From: Stefan Beller @ 2017-12-19 22:26 UTC (permalink / raw) To: git; +Cc: jrnieder, Stefan Beller Keep the local branch name as the upstream branch name to avoid confusion. Signed-off-by: Stefan Beller <sbeller@google.com> --- t/lib-submodule-update.sh | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/t/lib-submodule-update.sh b/t/lib-submodule-update.sh index 38dadd2c29..d7699046f6 100755 --- a/t/lib-submodule-update.sh +++ b/t/lib-submodule-update.sh @@ -664,8 +664,8 @@ test_submodule_recursing_with_args_common() { cd submodule_update && git -C sub1 checkout -b keep_branch && git -C sub1 rev-parse HEAD >expect && - git branch -t check-keep origin/modify_sub1 && - $command check-keep && + git branch -t modify_sub1 origin/modify_sub1 && + $command modify_sub1 && test_superproject_content origin/modify_sub1 && test_submodule_content sub1 origin/modify_sub1 && git -C sub1 rev-parse keep_branch >actual && -- 2.15.1.620.gb9897f4670-goog ^ permalink raw reply related [flat|nested] 38+ messages in thread
* [PATCH 2/5] t/lib-submodule-update.sh: Fix test ignoring ignored files in submodules 2017-12-19 22:26 [PATCH 0/5] Fix --recurse-submodules for submodule worktree changes Stefan Beller 2017-12-19 22:26 ` [PATCH 1/5] t/lib-submodule-update.sh: clarify test Stefan Beller @ 2017-12-19 22:26 ` Stefan Beller 2017-12-22 19:34 ` Junio C Hamano 2017-12-19 22:26 ` [PATCH 3/5] t/lib-submodule-update.sh: add new test for submodule internal change Stefan Beller ` (3 subsequent siblings) 5 siblings, 1 reply; 38+ messages in thread From: Stefan Beller @ 2017-12-19 22:26 UTC (permalink / raw) To: git; +Cc: jrnieder, Stefan Beller It turns out that this buggy test passed due to the buggy implementation, which will soon be corrected. Let's fix the test first. Signed-off-by: Stefan Beller <sbeller@google.com> --- t/lib-submodule-update.sh | 1 + 1 file changed, 1 insertion(+) diff --git a/t/lib-submodule-update.sh b/t/lib-submodule-update.sh index d7699046f6..fb0173ea87 100755 --- a/t/lib-submodule-update.sh +++ b/t/lib-submodule-update.sh @@ -885,6 +885,7 @@ test_submodule_switch_recursing_with_args () { ( cd submodule_update && git branch -t replace_sub1_with_file origin/replace_sub1_with_file && + echo ignored >.git/modules/sub1/info/exclude && : >sub1/ignored && $command replace_sub1_with_file && test_superproject_content origin/replace_sub1_with_file && -- 2.15.1.620.gb9897f4670-goog ^ permalink raw reply related [flat|nested] 38+ messages in thread
* Re: [PATCH 2/5] t/lib-submodule-update.sh: Fix test ignoring ignored files in submodules 2017-12-19 22:26 ` [PATCH 2/5] t/lib-submodule-update.sh: Fix test ignoring ignored files in submodules Stefan Beller @ 2017-12-22 19:34 ` Junio C Hamano 2017-12-27 19:27 ` Stefan Beller 0 siblings, 1 reply; 38+ messages in thread From: Junio C Hamano @ 2017-12-22 19:34 UTC (permalink / raw) To: Stefan Beller; +Cc: git, jrnieder Stefan Beller <sbeller@google.com> writes: > It turns out that this buggy test passed due to the buggy implementation, > which will soon be corrected. Let's fix the test first. Please clarify "buggy test". Without the original discussion (I am imagining there is something that happened on the list recently), here is my guess: We tried to make sure that an ignored file is $distimmed by $gostak, but forgot to tell the exclude mechanism that the path 'sub1'ignored' we use for the test is actually ignored, so the fact that the file was not $distimmed when $gostak did its thing meant nothing---mark any path whose name is 'ignored' as ignored to really test the condition we want to test. but I do not exactly know what verb to replace $distim and what noun to replace $gostak above ;-) Also, wouldn't this expose a bug in the implementation if that is the case? Thanks. > > Signed-off-by: Stefan Beller <sbeller@google.com> > --- > t/lib-submodule-update.sh | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/t/lib-submodule-update.sh b/t/lib-submodule-update.sh > index d7699046f6..fb0173ea87 100755 > --- a/t/lib-submodule-update.sh > +++ b/t/lib-submodule-update.sh > @@ -885,6 +885,7 @@ test_submodule_switch_recursing_with_args () { > ( > cd submodule_update && > git branch -t replace_sub1_with_file origin/replace_sub1_with_file && > + echo ignored >.git/modules/sub1/info/exclude && > : >sub1/ignored && > $command replace_sub1_with_file && > test_superproject_content origin/replace_sub1_with_file && ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH 2/5] t/lib-submodule-update.sh: Fix test ignoring ignored files in submodules 2017-12-22 19:34 ` Junio C Hamano @ 2017-12-27 19:27 ` Stefan Beller 0 siblings, 0 replies; 38+ messages in thread From: Stefan Beller @ 2017-12-27 19:27 UTC (permalink / raw) To: Junio C Hamano; +Cc: git, Jonathan Nieder On Fri, Dec 22, 2017 at 11:34 AM, Junio C Hamano <gitster@pobox.com> wrote: > Stefan Beller <sbeller@google.com> writes: > >> It turns out that this buggy test passed due to the buggy implementation, >> which will soon be corrected. Let's fix the test first. > > Please clarify "buggy test". Without the original discussion (I am > imagining there is something that happened on the list recently), > here is my guess: > > We tried to make sure that an ignored file is $distimmed by > $gostak, but forgot to tell the exclude mechanism that the path > 'sub1'ignored' we use for the test is actually ignored, so the > fact that the file was not $distimmed when $gostak did its thing > meant nothing---mark any path whose name is 'ignored' as ignored > to really test the condition we want to test. > > but I do not exactly know what verb to replace $distim and what noun > to replace $gostak above ;-) Just rewrote that commit message without distims and gostaking. > Also, wouldn't this expose a bug in the implementation if that is > the case? Yes it is, which will be fixed. The bug is: If the submodule HEAD is at the superprojects recorded object name, we don't bother looking into the submodule at all, neither for deleting untracked files, nor resetting changed tracked files. Thanks, Stefan ^ permalink raw reply [flat|nested] 38+ messages in thread
* [PATCH 3/5] t/lib-submodule-update.sh: add new test for submodule internal change 2017-12-19 22:26 [PATCH 0/5] Fix --recurse-submodules for submodule worktree changes Stefan Beller 2017-12-19 22:26 ` [PATCH 1/5] t/lib-submodule-update.sh: clarify test Stefan Beller 2017-12-19 22:26 ` [PATCH 2/5] t/lib-submodule-update.sh: Fix test ignoring ignored files in submodules Stefan Beller @ 2017-12-19 22:26 ` Stefan Beller 2017-12-19 22:31 ` Jonathan Nieder 2017-12-19 22:26 ` [PATCH 4/5] unpack-trees: Fix same() for submodules Stefan Beller ` (2 subsequent siblings) 5 siblings, 1 reply; 38+ messages in thread From: Stefan Beller @ 2017-12-19 22:26 UTC (permalink / raw) To: git; +Cc: jrnieder, Stefan Beller The test is marked as a failure as the fix comes in a later patch. Signed-off-by: Stefan Beller <sbeller@google.com> --- t/lib-submodule-update.sh | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/t/lib-submodule-update.sh b/t/lib-submodule-update.sh index fb0173ea87..15cf3e0b8b 100755 --- a/t/lib-submodule-update.sh +++ b/t/lib-submodule-update.sh @@ -1015,4 +1015,15 @@ test_submodule_forced_switch_recursing_with_args () { test_submodule_content sub1 origin/modify_sub1 ) ' + + test_expect_failure "$command: changed submodule worktree is reset" ' + prolog && + reset_work_tree_to_interested add_sub1 && + ( + cd submodule_update && + rm sub1/file1 && + $command HEAD && + test_path_is_file sub1/file1 + ) + ' } -- 2.15.1.620.gb9897f4670-goog ^ permalink raw reply related [flat|nested] 38+ messages in thread
* Re: [PATCH 3/5] t/lib-submodule-update.sh: add new test for submodule internal change 2017-12-19 22:26 ` [PATCH 3/5] t/lib-submodule-update.sh: add new test for submodule internal change Stefan Beller @ 2017-12-19 22:31 ` Jonathan Nieder 2017-12-19 22:35 ` Stefan Beller 0 siblings, 1 reply; 38+ messages in thread From: Jonathan Nieder @ 2017-12-19 22:31 UTC (permalink / raw) To: Stefan Beller; +Cc: git Hi, Stefan Beller wrote: > The test is marked as a failure as the fix comes in a later patch. > > Signed-off-by: Stefan Beller <sbeller@google.com> > --- > t/lib-submodule-update.sh | 11 +++++++++++ > 1 file changed, 11 insertions(+) I think I'd find this easier to undrestand if it were squashed with the patch that fixes it. This is part of test_submodule_foced_switch --- does that mean it affects both checkout -f and reset --hard, or only the latter? Thanks, Jonathan ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH 3/5] t/lib-submodule-update.sh: add new test for submodule internal change 2017-12-19 22:31 ` Jonathan Nieder @ 2017-12-19 22:35 ` Stefan Beller 0 siblings, 0 replies; 38+ messages in thread From: Stefan Beller @ 2017-12-19 22:35 UTC (permalink / raw) To: Jonathan Nieder; +Cc: git On Tue, Dec 19, 2017 at 2:31 PM, Jonathan Nieder <jrnieder@gmail.com> wrote: > Hi, > > Stefan Beller wrote: > >> The test is marked as a failure as the fix comes in a later patch. >> >> Signed-off-by: Stefan Beller <sbeller@google.com> >> --- >> t/lib-submodule-update.sh | 11 +++++++++++ >> 1 file changed, 11 insertions(+) > > I think I'd find this easier to undrestand if it were squashed with the > patch that fixes it. ok, let's squash this into the last patch then. > This is part of test_submodule_foced_switch --- does that mean it > affects both checkout -f and reset --hard, or only the latter? All of checkout -f reset --hard (and not reset --keep ;) read-tree -u --reset Thanks, Stefan ^ permalink raw reply [flat|nested] 38+ messages in thread
* [PATCH 4/5] unpack-trees: Fix same() for submodules 2017-12-19 22:26 [PATCH 0/5] Fix --recurse-submodules for submodule worktree changes Stefan Beller ` (2 preceding siblings ...) 2017-12-19 22:26 ` [PATCH 3/5] t/lib-submodule-update.sh: add new test for submodule internal change Stefan Beller @ 2017-12-19 22:26 ` Stefan Beller 2017-12-19 22:26 ` [PATCH 5/5] submodule: submodule_move_head omits old argument in forced case Stefan Beller 2017-12-27 22:57 ` [PATCHv2 0/5] Fix --recurse-submodules for submodule worktree changes Stefan Beller 5 siblings, 0 replies; 38+ messages in thread From: Stefan Beller @ 2017-12-19 22:26 UTC (permalink / raw) To: git; +Cc: jrnieder, Stefan Beller The function same(a, b) is used to check if two entries a and b are the same. As the index contains the staged files the same() function can be used to check if files between a given revision and the index are the same. In case of submodules, the gitlink entry is showing up as not modified despite potential changes inside the submodule. Fix the function to examine submodules by looking inside the submodule. This patch alone doesn't affect any correctness garantuees, but in combination with the next patch this fixes the new test introduced earlier in this series. This may be a slight performance regression as now we have to inspect any submodule thouroughly. Signed-off-by: Stefan Beller <sbeller@google.com> --- unpack-trees.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/unpack-trees.c b/unpack-trees.c index bf8b602901..4d839e8edb 100644 --- a/unpack-trees.c +++ b/unpack-trees.c @@ -1427,6 +1427,8 @@ static int same(const struct cache_entry *a, const struct cache_entry *b) return 1; if ((a->ce_flags | b->ce_flags) & CE_CONFLICTED) return 0; + if (S_ISGITLINK(b->ce_mode) && should_update_submodules()) + return !oidcmp(&a->oid, &b->oid) && !is_submodule_modified(b->name, 0); return a->ce_mode == b->ce_mode && !oidcmp(&a->oid, &b->oid); } -- 2.15.1.620.gb9897f4670-goog ^ permalink raw reply related [flat|nested] 38+ messages in thread
* [PATCH 5/5] submodule: submodule_move_head omits old argument in forced case 2017-12-19 22:26 [PATCH 0/5] Fix --recurse-submodules for submodule worktree changes Stefan Beller ` (3 preceding siblings ...) 2017-12-19 22:26 ` [PATCH 4/5] unpack-trees: Fix same() for submodules Stefan Beller @ 2017-12-19 22:26 ` Stefan Beller 2017-12-19 22:44 ` Jonathan Nieder 2017-12-27 22:57 ` [PATCHv2 0/5] Fix --recurse-submodules for submodule worktree changes Stefan Beller 5 siblings, 1 reply; 38+ messages in thread From: Stefan Beller @ 2017-12-19 22:26 UTC (permalink / raw) To: git; +Cc: jrnieder, Stefan Beller With the previous patch applied (fix of the same() function), the function `submodule_move_head` may be invoked with the same argument for the `old` and `new` state of a submodule, for example when you run `reset --hard --recurse-submodules` in the superproject that has no change in the gitlink entry, but only worktree related change in the submodule. The read-tree call in the submodule is not amused about the duplicate argument. It turns out that we can omit the duplicate old argument in all forced cases anyway, so let's do that. Signed-off-by: Stefan Beller <sbeller@google.com> --- submodule.c | 4 +++- t/lib-submodule-update.sh | 2 +- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/submodule.c b/submodule.c index fa25888783..db0f7ac51e 100644 --- a/submodule.c +++ b/submodule.c @@ -1653,7 +1653,9 @@ int submodule_move_head(const char *path, else argv_array_push(&cp.args, "-m"); - argv_array_push(&cp.args, old ? old : EMPTY_TREE_SHA1_HEX); + if (!(flags & SUBMODULE_MOVE_HEAD_FORCE)) + argv_array_push(&cp.args, old ? old : EMPTY_TREE_SHA1_HEX); + argv_array_push(&cp.args, new ? new : EMPTY_TREE_SHA1_HEX); if (run_command(&cp)) { diff --git a/t/lib-submodule-update.sh b/t/lib-submodule-update.sh index 15cf3e0b8b..7b6661cc84 100755 --- a/t/lib-submodule-update.sh +++ b/t/lib-submodule-update.sh @@ -1016,7 +1016,7 @@ test_submodule_forced_switch_recursing_with_args () { ) ' - test_expect_failure "$command: changed submodule worktree is reset" ' + test_expect_success "$command: changed submodule worktree is reset" ' prolog && reset_work_tree_to_interested add_sub1 && ( -- 2.15.1.620.gb9897f4670-goog ^ permalink raw reply related [flat|nested] 38+ messages in thread
* Re: [PATCH 5/5] submodule: submodule_move_head omits old argument in forced case 2017-12-19 22:26 ` [PATCH 5/5] submodule: submodule_move_head omits old argument in forced case Stefan Beller @ 2017-12-19 22:44 ` Jonathan Nieder 2017-12-19 22:54 ` Stefan Beller 0 siblings, 1 reply; 38+ messages in thread From: Jonathan Nieder @ 2017-12-19 22:44 UTC (permalink / raw) To: Stefan Beller; +Cc: git Hi, I had trouble understanding what this fixes, so I'll try nitpicking a bit as a sideways way to address that. Stefan Beller wrote: > With the previous patch applied (fix of the same() function), This tripped me up a bit. Usually commits assume that all previous patches have already been applied, since that allows the maintainer to apply the early part of a series on one day and the later part another day without losing too much context. I think this intends to say something like Now that we allow recursing into an unchanged submodule (see "unpack-trees: Fix same() for submodules", 2017-12-19), it is possible for ... > the > function `submodule_move_head` may be invoked with the same argument > for the `old` and `new` state of a submodule, for example when you > run `reset --hard --recurse-submodules` in the superproject that has no > change in the gitlink entry, but only worktree related change in the > submodule. The read-tree call in the submodule is not amused about > the duplicate argument. What is the symptom of read-tree being unamused? Is that a sign that these patches are out of order (i.e. that we should prepare to handle an unchanged submodule first, and then adjust the caller to pass in unchanged submodules)? > It turns out that we can omit the duplicate old argument in all forced > cases anyway, so let's do that. What is the end-user visibile effect of such a change? E.g. what becomes possible to a user that wasn't possible before? Among the commands you mentioned before: checkout -f I think I would expect this not to touch a submodule that hasn't changed, since that would be consistent with its behavior on files that haven't changed. reset --hard Nice! Yes, recursing into unchanged submodules is a big part of the psychological comfort of being able to say "you can always use reset --hard <commit> to get back to a known state". read-tree -u --reset This is essentially the plumbing equivalent of reset --hard, so it makes sense for them to be consistent. Ok. Based on the checkout -f case, if I've understood correctly then patch 4/5 goes too far on it (but I could easily be convinced otherwise). > Signed-off-by: Stefan Beller <sbeller@google.com> > --- > submodule.c | 4 +++- > t/lib-submodule-update.sh | 2 +- > 2 files changed, 4 insertions(+), 2 deletions(-) > > diff --git a/submodule.c b/submodule.c > index fa25888783..db0f7ac51e 100644 > --- a/submodule.c > +++ b/submodule.c > @@ -1653,7 +1653,9 @@ int submodule_move_head(const char *path, > else > argv_array_push(&cp.args, "-m"); > > - argv_array_push(&cp.args, old ? old : EMPTY_TREE_SHA1_HEX); > + if (!(flags & SUBMODULE_MOVE_HEAD_FORCE)) > + argv_array_push(&cp.args, old ? old : EMPTY_TREE_SHA1_HEX); Interesting. What is the effect when old != new? Thanks, Jonathan ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH 5/5] submodule: submodule_move_head omits old argument in forced case 2017-12-19 22:44 ` Jonathan Nieder @ 2017-12-19 22:54 ` Stefan Beller 2017-12-19 23:15 ` Jonathan Nieder 2017-12-20 0:01 ` Jonathan Nieder 0 siblings, 2 replies; 38+ messages in thread From: Stefan Beller @ 2017-12-19 22:54 UTC (permalink / raw) To: Jonathan Nieder; +Cc: git On Tue, Dec 19, 2017 at 2:44 PM, Jonathan Nieder <jrnieder@gmail.com> wrote: > Hi, > > I had trouble understanding what this fixes, so I'll try nitpicking a > bit as a sideways way to address that. > > Stefan Beller wrote: > >> With the previous patch applied (fix of the same() function), > > This tripped me up a bit. Usually commits assume that all previous > patches have already been applied, since that allows the maintainer to > apply the early part of a series on one day and the later part another > day without losing too much context. > > I think this intends to say something like > > Now that we allow recursing into an unchanged submodule (see > "unpack-trees: Fix same() for submodules", 2017-12-19), it is > possible for ... yup > >> the >> function `submodule_move_head` may be invoked with the same argument >> for the `old` and `new` state of a submodule, for example when you >> run `reset --hard --recurse-submodules` in the superproject that has no >> change in the gitlink entry, but only worktree related change in the >> submodule. The read-tree call in the submodule is not amused about >> the duplicate argument. > > What is the symptom of read-tree being unamused? Is that a sign that > these patches are out of order (i.e. that we should prepare to handle an > unchanged submodule first, and then adjust the caller to pass in > unchanged submodules)? > >> It turns out that we can omit the duplicate old argument in all forced >> cases anyway, so let's do that. > > What is the end-user visibile effect of such a change? E.g. what > becomes possible to a user that wasn't possible before? > > Among the commands you mentioned before: > > checkout -f > I think I would expect this not to touch a submodule that > hasn't changed, since that would be consistent with its > behavior on files that haven't changed. > > reset --hard > Nice! Yes, recursing into unchanged submodules is a big part > of the psychological comfort of being able to say "you can > always use reset --hard <commit> to get back to a known > state". I may have a different understanding of git commands than you do, but a plain "checkout -f" with no further arguments is the same as a hard reset IMHO, and could be used interchangeably? Rehashing the "What is a submodule?" discussion, I would claim that when its worktree is changed, we'd want checkout to restore the submodules worktree back, so I disagree with your assessment of checkout -f. > read-tree -u --reset > This is essentially the plumbing equivalent of reset --hard, > so it makes sense for them to be consistent. Ok. > > Based on the checkout -f case, if I've understood correctly then patch > 4/5 goes too far on it (but I could easily be convinced otherwise). Without 4/5 we cannot implement hard reset recursing into submodules as it is functionally the same as forced checkout except when we differentiate them on a higher layer? >> Signed-off-by: Stefan Beller <sbeller@google.com> >> --- >> submodule.c | 4 +++- >> t/lib-submodule-update.sh | 2 +- >> 2 files changed, 4 insertions(+), 2 deletions(-) >> >> diff --git a/submodule.c b/submodule.c >> index fa25888783..db0f7ac51e 100644 >> --- a/submodule.c >> +++ b/submodule.c >> @@ -1653,7 +1653,9 @@ int submodule_move_head(const char *path, >> else >> argv_array_push(&cp.args, "-m"); >> >> - argv_array_push(&cp.args, old ? old : EMPTY_TREE_SHA1_HEX); >> + if (!(flags & SUBMODULE_MOVE_HEAD_FORCE)) >> + argv_array_push(&cp.args, old ? old : EMPTY_TREE_SHA1_HEX); > > Interesting. What is the effect when old != new? when the force flag is set we mostly pass in old="HEAD", which is technically correct, but not a sha1. (I assume you want to know what happens when two unequal sha1s are given; for that it will perform a 2 way merge instead of a complete reset) Thanks, Stefan ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH 5/5] submodule: submodule_move_head omits old argument in forced case 2017-12-19 22:54 ` Stefan Beller @ 2017-12-19 23:15 ` Jonathan Nieder 2017-12-20 0:01 ` Jonathan Nieder 1 sibling, 0 replies; 38+ messages in thread From: Jonathan Nieder @ 2017-12-19 23:15 UTC (permalink / raw) To: Stefan Beller; +Cc: git Hi, Stefan Beller wrote: > On Tue, Dec 19, 2017 at 2:44 PM, Jonathan Nieder <jrnieder@gmail.com> wrote: >> checkout -f >> I think I would expect this not to touch a submodule that >> hasn't changed, since that would be consistent with its >> behavior on files that haven't changed. [...] > I may have a different understanding of git commands than you do, > but a plain "checkout -f" with no further arguments is the same as > a hard reset IMHO, and could be used interchangeably? Ah, good catch. Filed https://crbug.com/git/5 to clarify this in documentation. Thanks for clarifying. Jonathan ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH 5/5] submodule: submodule_move_head omits old argument in forced case 2017-12-19 22:54 ` Stefan Beller 2017-12-19 23:15 ` Jonathan Nieder @ 2017-12-20 0:01 ` Jonathan Nieder 2017-12-20 19:36 ` Stefan Beller 1 sibling, 1 reply; 38+ messages in thread From: Jonathan Nieder @ 2017-12-20 0:01 UTC (permalink / raw) To: Stefan Beller; +Cc: git Stefan Beller wrote: > On Tue, Dec 19, 2017 at 2:44 PM, Jonathan Nieder <jrnieder@gmail.com> wrote: >> checkout -f >> I think I would expect this not to touch a submodule that >> hasn't changed, since that would be consistent with its >> behavior on files that haven't changed. [...] > I may have a different understanding of git commands than you do, > but a plain "checkout -f" with no further arguments is the same as > a hard reset IMHO, and could be used interchangeably? A kind person pointed out privately that you were talking about "git checkout -f" with no further arguments, not "git checkout -f <commit>". In that context, the competing meanings I mentioned in https://crbug.com/git/5 don't exist: either a given entry in the worktree matches the index or it doesn't. So plain "git checkout -f" is similar to plain "git reset --hard" in that it means "make the worktree (and index, in the reset case) look just like this". checkout -f makes the worktree look like the index; reset --hard makes the worktree and index look like HEAD. In that context, more aggressively making the submodule in the worktree get into the defined state sounds to me like a good change. Hopefully my confusion helps illustrate what a commit message focusing on the end user experience might want to mention. Thanks again, Jonathan ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH 5/5] submodule: submodule_move_head omits old argument in forced case 2017-12-20 0:01 ` Jonathan Nieder @ 2017-12-20 19:36 ` Stefan Beller 2017-12-20 19:38 ` Stefan Beller 0 siblings, 1 reply; 38+ messages in thread From: Stefan Beller @ 2017-12-20 19:36 UTC (permalink / raw) To: Jonathan Nieder; +Cc: git On Tue, Dec 19, 2017 at 4:01 PM, Jonathan Nieder <jrnieder@gmail.com> wrote: > Stefan Beller wrote: >> On Tue, Dec 19, 2017 at 2:44 PM, Jonathan Nieder <jrnieder@gmail.com> wrote: > >>> checkout -f >>> I think I would expect this not to touch a submodule that >>> hasn't changed, since that would be consistent with its >>> behavior on files that haven't changed. > [...] >> I may have a different understanding of git commands than you do, >> but a plain "checkout -f" with no further arguments is the same as >> a hard reset IMHO, and could be used interchangeably? > > A kind person pointed out privately that you were talking about > "git checkout -f" with no further arguments, not "git checkout -f > <commit>". In that context, the competing meanings I mentioned in > https://crbug.com/git/5 don't exist: either a given entry in the > worktree matches the index or it doesn't. > > So plain "git checkout -f" is similar to plain "git reset --hard" > in that it means "make the worktree (and index, in the reset case) > look just like this". with "this" == the argument that was given, if the argument was omitted, HEAD is assumed. > checkout -f makes the worktree look like the index; No, here is what my installation of git (recent master) does: $ git --version git version 2.15.1.389.g52015aaf9d $ cat test.sh rm -rf tmp git init tmp cd tmp git commit --allow-empty -m initial echo commit >a echo commit >b echo commit >c git add . git commit -m commit echo index >a git add a echo worktree >a echo index >b git add b echo worktree>c $ sh test.sh Initialized empty Git repository in /u/tmp/.git/ [master (root-commit) c109fb5] initial [master fcc21ea] commit 3 files changed, 3 insertions(+) create mode 100644 a create mode 100644 b create mode 100644 c $ cd tmp $ git status On branch master Changes to be committed: (use "git reset HEAD <file>..." to unstage) modified: a modified: b Changes not staged for commit: (use "git add <file>..." to update what will be committed) (use "git checkout -- <file>..." to discard changes in working directory) modified: a modified: c $ git checkout -f $ git status On branch master nothing to commit, working tree clean # Let's test the other commands as well: $ cd .. $ sh test.sh ... $ cd tmp $ git checkout -f HEAD $ git status On branch master nothing to commit, working tree clean # See, there is no difference between giving HEAD as an argument # or not! Try again with reset just for completeness: $ cd .. $ sh test.sh ... $ cd tmp $ git reset --hard HEAD is now at b71ae70 commit $ git status On branch master nothing to commit, working tree clean # The only difference between reset and the checkout is that reset # tells me where we are. > reset --hard makes the worktree and index look like HEAD. I agree. > In that context, more aggressively making the submodule in the > worktree get into the defined state sounds to me like a good change. > > Hopefully my confusion helps illustrate what a commit message focusing > on the end user experience might want to mention. I'll try to come up with a better commit message. Thanks for bearing with me here. Stefan > > Thanks again, > Jonathan ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH 5/5] submodule: submodule_move_head omits old argument in forced case 2017-12-20 19:36 ` Stefan Beller @ 2017-12-20 19:38 ` Stefan Beller 0 siblings, 0 replies; 38+ messages in thread From: Stefan Beller @ 2017-12-20 19:38 UTC (permalink / raw) To: Jonathan Nieder; +Cc: git On Wed, Dec 20, 2017 at 11:36 AM, Stefan Beller <sbeller@google.com> wrote: > On Tue, Dec 19, 2017 at 4:01 PM, Jonathan Nieder <jrnieder@gmail.com> wrote: >> Stefan Beller wrote: >>> On Tue, Dec 19, 2017 at 2:44 PM, Jonathan Nieder <jrnieder@gmail.com> wrote: >> >>>> checkout -f >>>> I think I would expect this not to touch a submodule that >>>> hasn't changed, since that would be consistent with its >>>> behavior on files that haven't changed. >> [...] >>> I may have a different understanding of git commands than you do, >>> but a plain "checkout -f" with no further arguments is the same as >>> a hard reset IMHO, and could be used interchangeably? >> >> A kind person pointed out privately that you were talking about >> "git checkout -f" with no further arguments, not "git checkout -f >> <commit>". In that context, the competing meanings I mentioned in >> https://crbug.com/git/5 don't exist: either a given entry in the >> worktree matches the index or it doesn't. >> >> So plain "git checkout -f" is similar to plain "git reset --hard" >> in that it means "make the worktree (and index, in the reset case) >> look just like this". > > with "this" == the argument that was given, if the argument > was omitted, HEAD is assumed. > >> checkout -f makes the worktree look like the index; > > No, here is what my installation of git (recent master) does: Well, you are technically correct, the worktree is made look like the index, but prior to that the index is reset to the HEAD, so for me it is easier to understand as "make worktree and index like HEAD" ^ permalink raw reply [flat|nested] 38+ messages in thread
* [PATCHv2 0/5] Fix --recurse-submodules for submodule worktree changes 2017-12-19 22:26 [PATCH 0/5] Fix --recurse-submodules for submodule worktree changes Stefan Beller ` (4 preceding siblings ...) 2017-12-19 22:26 ` [PATCH 5/5] submodule: submodule_move_head omits old argument in forced case Stefan Beller @ 2017-12-27 22:57 ` Stefan Beller 2017-12-27 22:57 ` [PATCHv2 1/5] t/helper/test-lazy-name-hash: fix compilation Stefan Beller ` (4 more replies) 5 siblings, 5 replies; 38+ messages in thread From: Stefan Beller @ 2017-12-27 22:57 UTC (permalink / raw) To: sbeller; +Cc: git, jrnieder I dropped the patch to `same()` as I realized we only need to fix the oneway_merge function, the others (two, three way merge) are fine as they have the checks already in place. The test added in the last patch got slightly larger as now we also test for newly staged files to be blown away in the submodule. v1: The fix is in the last patch, the first patches are just massaging the code base to make the fix easy. The second patch fixes a bug in the test, which was ineffective at testing. The third patch shows the problem this series addresses, the fourth patch is a little refactoring, which I want to keep separate as I would expect it to be a performance regression[1]. The first patch is unrelated, but improves the readability of submodule test cases, which we'd want to improve further. Thanks, Stefan Stefan Beller (5): t/helper/test-lazy-name-hash: fix compilation t/lib-submodule-update.sh: clarify test t/lib-submodule-update.sh: Fix test ignoring ignored files in submodules unpack-trees: oneway_merge to update submodules submodule: submodule_move_head omits old argument in forced case submodule.c | 4 +++- t/helper/test-lazy-init-name-hash.c | 2 +- t/lib-submodule-update.sh | 19 +++++++++++++++++-- unpack-trees.c | 3 +++ 4 files changed, 24 insertions(+), 4 deletions(-) -- 2.15.1.620.gb9897f4670-goog ^ permalink raw reply [flat|nested] 38+ messages in thread
* [PATCHv2 1/5] t/helper/test-lazy-name-hash: fix compilation 2017-12-27 22:57 ` [PATCHv2 0/5] Fix --recurse-submodules for submodule worktree changes Stefan Beller @ 2017-12-27 22:57 ` Stefan Beller 2017-12-27 22:57 ` [PATCHv2 2/5] t/lib-submodule-update.sh: clarify test Stefan Beller ` (3 subsequent siblings) 4 siblings, 0 replies; 38+ messages in thread From: Stefan Beller @ 2017-12-27 22:57 UTC (permalink / raw) To: sbeller; +Cc: git, jrnieder I was compiling origin/master today with stricter compiler flags today and was greeted by t/helper/test-lazy-init-name-hash.c: In function ‘cmd_main’: t/helper/test-lazy-init-name-hash.c:172:5: error: ‘nr_threads_used’ may be used uninitialized in this function [-Werror=maybe-uninitialized] printf("avg [size %8d] [single %f] %c [multi %f %d]\n", ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ nr, ~~~ (double)avg_single/1000000000, ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ (avg_single < avg_multi ? '<' : '>'), ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ (double)avg_multi/1000000000, ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ nr_threads_used); ~~~~~~~~~~~~~~~~ t/helper/test-lazy-init-name-hash.c:115:6: note: ‘nr_threads_used’ was declared here int nr_threads_used; ^~~~~~~~~~~~~~~ I do not see how we can arrive at that line without having `nr_threads_used` initialized, as we'd have `count > 1` (which asserts that we ran the loop above at least once, such that it *should* be initialized). I do not have time to dive into further analysis. Signed-off-by: Stefan Beller <sbeller@google.com> --- t/helper/test-lazy-init-name-hash.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/t/helper/test-lazy-init-name-hash.c b/t/helper/test-lazy-init-name-hash.c index 6368a89345..297fb01d61 100644 --- a/t/helper/test-lazy-init-name-hash.c +++ b/t/helper/test-lazy-init-name-hash.c @@ -112,7 +112,7 @@ static void analyze_run(void) { uint64_t t1s, t1m, t2s, t2m; int cache_nr_limit; - int nr_threads_used; + int nr_threads_used = 0; int i; int nr; -- 2.15.1.620.gb9897f4670-goog ^ permalink raw reply related [flat|nested] 38+ messages in thread
* [PATCHv2 2/5] t/lib-submodule-update.sh: clarify test 2017-12-27 22:57 ` [PATCHv2 0/5] Fix --recurse-submodules for submodule worktree changes Stefan Beller 2017-12-27 22:57 ` [PATCHv2 1/5] t/helper/test-lazy-name-hash: fix compilation Stefan Beller @ 2017-12-27 22:57 ` Stefan Beller 2017-12-27 22:57 ` [PATCHv2 3/5] t/lib-submodule-update.sh: Fix test ignoring ignored files in submodules Stefan Beller ` (2 subsequent siblings) 4 siblings, 0 replies; 38+ messages in thread From: Stefan Beller @ 2017-12-27 22:57 UTC (permalink / raw) To: sbeller; +Cc: git, jrnieder Keep the local branch name as the upstream branch name to avoid confusion. Signed-off-by: Stefan Beller <sbeller@google.com> --- t/lib-submodule-update.sh | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/t/lib-submodule-update.sh b/t/lib-submodule-update.sh index 38dadd2c29..d7699046f6 100755 --- a/t/lib-submodule-update.sh +++ b/t/lib-submodule-update.sh @@ -664,8 +664,8 @@ test_submodule_recursing_with_args_common() { cd submodule_update && git -C sub1 checkout -b keep_branch && git -C sub1 rev-parse HEAD >expect && - git branch -t check-keep origin/modify_sub1 && - $command check-keep && + git branch -t modify_sub1 origin/modify_sub1 && + $command modify_sub1 && test_superproject_content origin/modify_sub1 && test_submodule_content sub1 origin/modify_sub1 && git -C sub1 rev-parse keep_branch >actual && -- 2.15.1.620.gb9897f4670-goog ^ permalink raw reply related [flat|nested] 38+ messages in thread
* [PATCHv2 3/5] t/lib-submodule-update.sh: Fix test ignoring ignored files in submodules 2017-12-27 22:57 ` [PATCHv2 0/5] Fix --recurse-submodules for submodule worktree changes Stefan Beller 2017-12-27 22:57 ` [PATCHv2 1/5] t/helper/test-lazy-name-hash: fix compilation Stefan Beller 2017-12-27 22:57 ` [PATCHv2 2/5] t/lib-submodule-update.sh: clarify test Stefan Beller @ 2017-12-27 22:57 ` Stefan Beller 2017-12-27 22:57 ` [PATCHv2 4/5] unpack-trees: oneway_merge to update submodules Stefan Beller 2017-12-27 22:57 ` [PATCHv2 5/5] submodule: submodule_move_head omits old argument in forced case Stefan Beller 4 siblings, 0 replies; 38+ messages in thread From: Stefan Beller @ 2017-12-27 22:57 UTC (permalink / raw) To: sbeller; +Cc: git, jrnieder It turns out that the test replacing a submodule with a file with the submodule containing an ignored file is incorrectly titled, because the test put the file in place, but never ignored that file. When having an untracked file Instead of an ignored file in the submodule, git should refuse to remove the submodule, but that is a bug in the implementation of recursing into submodules, such that the test just passed, removing the untracked file. Fix the test first; in a later patch we'll fix gits behavior, that will make sure untracked files are not deleted. Signed-off-by: Stefan Beller <sbeller@google.com> --- t/lib-submodule-update.sh | 1 + 1 file changed, 1 insertion(+) diff --git a/t/lib-submodule-update.sh b/t/lib-submodule-update.sh index d7699046f6..fb0173ea87 100755 --- a/t/lib-submodule-update.sh +++ b/t/lib-submodule-update.sh @@ -885,6 +885,7 @@ test_submodule_switch_recursing_with_args () { ( cd submodule_update && git branch -t replace_sub1_with_file origin/replace_sub1_with_file && + echo ignored >.git/modules/sub1/info/exclude && : >sub1/ignored && $command replace_sub1_with_file && test_superproject_content origin/replace_sub1_with_file && -- 2.15.1.620.gb9897f4670-goog ^ permalink raw reply related [flat|nested] 38+ messages in thread
* [PATCHv2 4/5] unpack-trees: oneway_merge to update submodules 2017-12-27 22:57 ` [PATCHv2 0/5] Fix --recurse-submodules for submodule worktree changes Stefan Beller ` (2 preceding siblings ...) 2017-12-27 22:57 ` [PATCHv2 3/5] t/lib-submodule-update.sh: Fix test ignoring ignored files in submodules Stefan Beller @ 2017-12-27 22:57 ` Stefan Beller 2018-01-02 19:43 ` Junio C Hamano 2017-12-27 22:57 ` [PATCHv2 5/5] submodule: submodule_move_head omits old argument in forced case Stefan Beller 4 siblings, 1 reply; 38+ messages in thread From: Stefan Beller @ 2017-12-27 22:57 UTC (permalink / raw) To: sbeller; +Cc: git, jrnieder When there is a one way merge, each submodule needs to be one way merged as well, if we're asked to recurse into submodules. In case of a submodule, check if it is up-to-date, otherwise set the flag CE_UPDATE, which will trigger an update of it in the phase updating the tree later. Signed-off-by: Stefan Beller <sbeller@google.com> --- unpack-trees.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/unpack-trees.c b/unpack-trees.c index bf8b602901..ac59524a7f 100644 --- a/unpack-trees.c +++ b/unpack-trees.c @@ -2139,6 +2139,9 @@ int oneway_merge(const struct cache_entry * const *src, ie_match_stat(o->src_index, old, &st, CE_MATCH_IGNORE_VALID|CE_MATCH_IGNORE_SKIP_WORKTREE)) update |= CE_UPDATE; } + if (S_ISGITLINK(old->ce_mode) && should_update_submodules() && + !verify_uptodate(old, o)) + update |= CE_UPDATE; add_entry(o, old, update, 0); return 0; } -- 2.15.1.620.gb9897f4670-goog ^ permalink raw reply related [flat|nested] 38+ messages in thread
* Re: [PATCHv2 4/5] unpack-trees: oneway_merge to update submodules 2017-12-27 22:57 ` [PATCHv2 4/5] unpack-trees: oneway_merge to update submodules Stefan Beller @ 2018-01-02 19:43 ` Junio C Hamano 2018-01-02 23:04 ` Stefan Beller 2018-01-03 1:12 ` [PATCHv3 0/5] Fix --recurse-submodules for submodule worktree changes Stefan Beller 0 siblings, 2 replies; 38+ messages in thread From: Junio C Hamano @ 2018-01-02 19:43 UTC (permalink / raw) To: Stefan Beller; +Cc: git, jrnieder Stefan Beller <sbeller@google.com> writes: > diff --git a/unpack-trees.c b/unpack-trees.c > index bf8b602901..ac59524a7f 100644 > --- a/unpack-trees.c > +++ b/unpack-trees.c > @@ -2139,6 +2139,9 @@ int oneway_merge(const struct cache_entry * const *src, > ie_match_stat(o->src_index, old, &st, CE_MATCH_IGNORE_VALID|CE_MATCH_IGNORE_SKIP_WORKTREE)) > update |= CE_UPDATE; > } > + if (S_ISGITLINK(old->ce_mode) && should_update_submodules() && > + !verify_uptodate(old, o)) > + update |= CE_UPDATE; > add_entry(o, old, update, 0); > return 0; > } This is more sensible than the previous one that broke same() by unconditionally checking the working tree state, even when the machinery is told not to look at the working tree. The code in the pre-context of this hunk, (i.e. the original one you are half-way mimicking), we realize that the original cache entry and the cache entry we are checking out are exactly the same and we overwrite when we know that the path in the working tree is dirty, and we are not told to "skip" that path in the working tree (i.e. sparse checkout). That happens only when we are told to o->update and o->reset. Shouldn't we be setting the update bit only when o->update is in effect, just like we can see in the pre-context of this hunk? I do not know if we need to require o->reset and/or need to pay attention to the skip worktree bit in this new case. ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCHv2 4/5] unpack-trees: oneway_merge to update submodules 2018-01-02 19:43 ` Junio C Hamano @ 2018-01-02 23:04 ` Stefan Beller 2018-01-03 1:12 ` [PATCHv3 0/5] Fix --recurse-submodules for submodule worktree changes Stefan Beller 1 sibling, 0 replies; 38+ messages in thread From: Stefan Beller @ 2018-01-02 23:04 UTC (permalink / raw) To: Junio C Hamano; +Cc: git, Jonathan Nieder On Tue, Jan 2, 2018 at 11:43 AM, Junio C Hamano <gitster@pobox.com> wrote: > Stefan Beller <sbeller@google.com> writes: > >> diff --git a/unpack-trees.c b/unpack-trees.c >> index bf8b602901..ac59524a7f 100644 >> --- a/unpack-trees.c >> +++ b/unpack-trees.c >> @@ -2139,6 +2139,9 @@ int oneway_merge(const struct cache_entry * const *src, >> ie_match_stat(o->src_index, old, &st, CE_MATCH_IGNORE_VALID|CE_MATCH_IGNORE_SKIP_WORKTREE)) >> update |= CE_UPDATE; >> } >> + if (S_ISGITLINK(old->ce_mode) && should_update_submodules() && >> + !verify_uptodate(old, o)) >> + update |= CE_UPDATE; >> add_entry(o, old, update, 0); >> return 0; >> } > > This is more sensible than the previous one that broke same() by > unconditionally checking the working tree state, even when the > machinery is told not to look at the working tree. > > The code in the pre-context of this hunk, (i.e. the original one you > are half-way mimicking), we realize that the original cache entry > and the cache entry we are checking out are exactly the same and we > overwrite when we know that the path in the working tree is dirty, > and we are not told to "skip" that path in the working tree > (i.e. sparse checkout). That happens only when we are told to > o->update and o->reset. > > Shouldn't we be setting the update bit only when o->update is in > effect, Yes, we should. > just like we can see in the pre-context of this hunk? I do > not know if we need to require o->reset and/or need to pay attention > to the skip worktree bit in this new case. verify_uptodate takes care of the worktree bits ("o->skip_sparse_checkout && (ce->ce_flags & CE_NEW_SKIP_WORKTREE))") o->reset is taken care of inside the nested verify_uptodate_1 function via ... else if (o->reset || ce_uptodate(ce)) return 0; So I'd think we only need to toss in the o->update check. And that additional check would only be a performance optimization as it would omit the check in case we do not want to update. (verify_uptodate is expensive for submodules) ^ permalink raw reply [flat|nested] 38+ messages in thread
* [PATCHv3 0/5] Fix --recurse-submodules for submodule worktree changes 2018-01-02 19:43 ` Junio C Hamano 2018-01-02 23:04 ` Stefan Beller @ 2018-01-03 1:12 ` Stefan Beller 2018-01-03 1:12 ` [PATCH 1/5] t/helper/test-lazy-name-hash: fix compilation Stefan Beller ` (5 more replies) 1 sibling, 6 replies; 38+ messages in thread From: Stefan Beller @ 2018-01-03 1:12 UTC (permalink / raw) To: gitster; +Cc: git, jrnieder, sbeller Thanks Junio for review of this series! The only change in this version of the series is --- a/unpack-trees.c +++ b/unpack-trees.c @@ -2140,7 +2140,7 @@ int oneway_merge(const struct cache_entry * const *src, update |= CE_UPDATE; } if (S_ISGITLINK(old->ce_mode) && should_update_submodules() && - !verify_uptodate(old, o)) + o->update && !verify_uptodate(old, o)) update |= CE_UPDATE; add_entry(o, old, update, 0); v2: I dropped the patch to `same()` as I realized we only need to fix the oneway_merge function, the others (two, three way merge) are fine as they have the checks already in place. The test added in the last patch got slightly larger as now we also test for newly staged files to be blown away in the submodule. v1: The fix is in the last patch, the first patches are just massaging the code base to make the fix easy. The second patch fixes a bug in the test, which was ineffective at testing. The third patch shows the problem this series addresses, the fourth patch is a little refactoring, which I want to keep separate as I would expect it to be a performance regression[1]. The first patch is unrelated, but improves the readability of submodule test cases, which we'd want to improve further. Thanks, Stefan Stefan Beller (5): t/helper/test-lazy-name-hash: fix compilation t/lib-submodule-update.sh: clarify test t/lib-submodule-update.sh: Fix test ignoring ignored files in submodules unpack-trees: oneway_merge to update submodules submodule: submodule_move_head omits old argument in forced case submodule.c | 4 +++- t/helper/test-lazy-init-name-hash.c | 2 +- t/lib-submodule-update.sh | 19 +++++++++++++++++-- unpack-trees.c | 3 +++ 4 files changed, 24 insertions(+), 4 deletions(-) -- 2.15.1.620.gb9897f4670-goog ^ permalink raw reply [flat|nested] 38+ messages in thread
* [PATCH 1/5] t/helper/test-lazy-name-hash: fix compilation 2018-01-03 1:12 ` [PATCHv3 0/5] Fix --recurse-submodules for submodule worktree changes Stefan Beller @ 2018-01-03 1:12 ` Stefan Beller 2018-01-03 1:12 ` [PATCH 2/5] t/lib-submodule-update.sh: clarify test Stefan Beller ` (4 subsequent siblings) 5 siblings, 0 replies; 38+ messages in thread From: Stefan Beller @ 2018-01-03 1:12 UTC (permalink / raw) To: gitster; +Cc: git, jrnieder, sbeller I was compiling origin/master today with stricter compiler flags today and was greeted by t/helper/test-lazy-init-name-hash.c: In function ‘cmd_main’: t/helper/test-lazy-init-name-hash.c:172:5: error: ‘nr_threads_used’ may be used uninitialized in this function [-Werror=maybe-uninitialized] printf("avg [size %8d] [single %f] %c [multi %f %d]\n", ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ nr, ~~~ (double)avg_single/1000000000, ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ (avg_single < avg_multi ? '<' : '>'), ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ (double)avg_multi/1000000000, ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ nr_threads_used); ~~~~~~~~~~~~~~~~ t/helper/test-lazy-init-name-hash.c:115:6: note: ‘nr_threads_used’ was declared here int nr_threads_used; ^~~~~~~~~~~~~~~ I do not see how we can arrive at that line without having `nr_threads_used` initialized, as we'd have `count > 1` (which asserts that we ran the loop above at least once, such that it *should* be initialized). I do not have time to dive into further analysis. Signed-off-by: Stefan Beller <sbeller@google.com> --- t/helper/test-lazy-init-name-hash.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/t/helper/test-lazy-init-name-hash.c b/t/helper/test-lazy-init-name-hash.c index 6368a89345..297fb01d61 100644 --- a/t/helper/test-lazy-init-name-hash.c +++ b/t/helper/test-lazy-init-name-hash.c @@ -112,7 +112,7 @@ static void analyze_run(void) { uint64_t t1s, t1m, t2s, t2m; int cache_nr_limit; - int nr_threads_used; + int nr_threads_used = 0; int i; int nr; -- 2.15.1.620.gb9897f4670-goog ^ permalink raw reply related [flat|nested] 38+ messages in thread
* [PATCH 2/5] t/lib-submodule-update.sh: clarify test 2018-01-03 1:12 ` [PATCHv3 0/5] Fix --recurse-submodules for submodule worktree changes Stefan Beller 2018-01-03 1:12 ` [PATCH 1/5] t/helper/test-lazy-name-hash: fix compilation Stefan Beller @ 2018-01-03 1:12 ` Stefan Beller 2018-01-03 1:12 ` [PATCH 3/5] t/lib-submodule-update.sh: Fix test ignoring ignored files in submodules Stefan Beller ` (3 subsequent siblings) 5 siblings, 0 replies; 38+ messages in thread From: Stefan Beller @ 2018-01-03 1:12 UTC (permalink / raw) To: gitster; +Cc: git, jrnieder, sbeller Keep the local branch name as the upstream branch name to avoid confusion. Signed-off-by: Stefan Beller <sbeller@google.com> --- t/lib-submodule-update.sh | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/t/lib-submodule-update.sh b/t/lib-submodule-update.sh index 38dadd2c29..d7699046f6 100755 --- a/t/lib-submodule-update.sh +++ b/t/lib-submodule-update.sh @@ -664,8 +664,8 @@ test_submodule_recursing_with_args_common() { cd submodule_update && git -C sub1 checkout -b keep_branch && git -C sub1 rev-parse HEAD >expect && - git branch -t check-keep origin/modify_sub1 && - $command check-keep && + git branch -t modify_sub1 origin/modify_sub1 && + $command modify_sub1 && test_superproject_content origin/modify_sub1 && test_submodule_content sub1 origin/modify_sub1 && git -C sub1 rev-parse keep_branch >actual && -- 2.15.1.620.gb9897f4670-goog ^ permalink raw reply related [flat|nested] 38+ messages in thread
* [PATCH 3/5] t/lib-submodule-update.sh: Fix test ignoring ignored files in submodules 2018-01-03 1:12 ` [PATCHv3 0/5] Fix --recurse-submodules for submodule worktree changes Stefan Beller 2018-01-03 1:12 ` [PATCH 1/5] t/helper/test-lazy-name-hash: fix compilation Stefan Beller 2018-01-03 1:12 ` [PATCH 2/5] t/lib-submodule-update.sh: clarify test Stefan Beller @ 2018-01-03 1:12 ` Stefan Beller 2018-01-03 1:12 ` [PATCH 4/5] unpack-trees: oneway_merge to update submodules Stefan Beller ` (2 subsequent siblings) 5 siblings, 0 replies; 38+ messages in thread From: Stefan Beller @ 2018-01-03 1:12 UTC (permalink / raw) To: gitster; +Cc: git, jrnieder, sbeller It turns out that the test replacing a submodule with a file with the submodule containing an ignored file is incorrectly titled, because the test put the file in place, but never ignored that file. When having an untracked file Instead of an ignored file in the submodule, git should refuse to remove the submodule, but that is a bug in the implementation of recursing into submodules, such that the test just passed, removing the untracked file. Fix the test first; in a later patch we'll fix gits behavior, that will make sure untracked files are not deleted. Signed-off-by: Stefan Beller <sbeller@google.com> --- t/lib-submodule-update.sh | 1 + 1 file changed, 1 insertion(+) diff --git a/t/lib-submodule-update.sh b/t/lib-submodule-update.sh index d7699046f6..fb0173ea87 100755 --- a/t/lib-submodule-update.sh +++ b/t/lib-submodule-update.sh @@ -885,6 +885,7 @@ test_submodule_switch_recursing_with_args () { ( cd submodule_update && git branch -t replace_sub1_with_file origin/replace_sub1_with_file && + echo ignored >.git/modules/sub1/info/exclude && : >sub1/ignored && $command replace_sub1_with_file && test_superproject_content origin/replace_sub1_with_file && -- 2.15.1.620.gb9897f4670-goog ^ permalink raw reply related [flat|nested] 38+ messages in thread
* [PATCH 4/5] unpack-trees: oneway_merge to update submodules 2018-01-03 1:12 ` [PATCHv3 0/5] Fix --recurse-submodules for submodule worktree changes Stefan Beller ` (2 preceding siblings ...) 2018-01-03 1:12 ` [PATCH 3/5] t/lib-submodule-update.sh: Fix test ignoring ignored files in submodules Stefan Beller @ 2018-01-03 1:12 ` Stefan Beller 2018-01-03 1:12 ` [PATCH 5/5] submodule: submodule_move_head omits old argument in forced case Stefan Beller 2018-01-03 20:49 ` [PATCHv3 0/5] Fix --recurse-submodules for submodule worktree changes Junio C Hamano 5 siblings, 0 replies; 38+ messages in thread From: Stefan Beller @ 2018-01-03 1:12 UTC (permalink / raw) To: gitster; +Cc: git, jrnieder, sbeller When there is a one way merge, each submodule needs to be one way merged as well, if we're asked to recurse into submodules. In case of a submodule, check if it is up-to-date, otherwise set the flag CE_UPDATE, which will trigger an update of it in the phase updating the tree later. Signed-off-by: Stefan Beller <sbeller@google.com> --- unpack-trees.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/unpack-trees.c b/unpack-trees.c index bf8b602901..7657d6ecdd 100644 --- a/unpack-trees.c +++ b/unpack-trees.c @@ -2139,6 +2139,9 @@ int oneway_merge(const struct cache_entry * const *src, ie_match_stat(o->src_index, old, &st, CE_MATCH_IGNORE_VALID|CE_MATCH_IGNORE_SKIP_WORKTREE)) update |= CE_UPDATE; } + if (S_ISGITLINK(old->ce_mode) && should_update_submodules() && + o->update && !verify_uptodate(old, o)) + update |= CE_UPDATE; add_entry(o, old, update, 0); return 0; } -- 2.15.1.620.gb9897f4670-goog ^ permalink raw reply related [flat|nested] 38+ messages in thread
* [PATCH 5/5] submodule: submodule_move_head omits old argument in forced case 2018-01-03 1:12 ` [PATCHv3 0/5] Fix --recurse-submodules for submodule worktree changes Stefan Beller ` (3 preceding siblings ...) 2018-01-03 1:12 ` [PATCH 4/5] unpack-trees: oneway_merge to update submodules Stefan Beller @ 2018-01-03 1:12 ` Stefan Beller 2018-01-03 20:49 ` [PATCHv3 0/5] Fix --recurse-submodules for submodule worktree changes Junio C Hamano 5 siblings, 0 replies; 38+ messages in thread From: Stefan Beller @ 2018-01-03 1:12 UTC (permalink / raw) To: gitster; +Cc: git, jrnieder, sbeller When using hard reset or forced checkout with the option to recurse into submodules, the submodules need to be reset, too. It turns out that we need to omit the duplicate old argument to read-tree in all forced cases to omit the 2 way merge and use the more assertive behavior of reading the specific new tree into the index and updating the working tree. Signed-off-by: Stefan Beller <sbeller@google.com> --- submodule.c | 4 +++- t/lib-submodule-update.sh | 14 ++++++++++++++ 2 files changed, 17 insertions(+), 1 deletion(-) diff --git a/submodule.c b/submodule.c index fa25888783..db0f7ac51e 100644 --- a/submodule.c +++ b/submodule.c @@ -1653,7 +1653,9 @@ int submodule_move_head(const char *path, else argv_array_push(&cp.args, "-m"); - argv_array_push(&cp.args, old ? old : EMPTY_TREE_SHA1_HEX); + if (!(flags & SUBMODULE_MOVE_HEAD_FORCE)) + argv_array_push(&cp.args, old ? old : EMPTY_TREE_SHA1_HEX); + argv_array_push(&cp.args, new ? new : EMPTY_TREE_SHA1_HEX); if (run_command(&cp)) { diff --git a/t/lib-submodule-update.sh b/t/lib-submodule-update.sh index fb0173ea87..1f38a85371 100755 --- a/t/lib-submodule-update.sh +++ b/t/lib-submodule-update.sh @@ -1015,4 +1015,18 @@ test_submodule_forced_switch_recursing_with_args () { test_submodule_content sub1 origin/modify_sub1 ) ' + + test_expect_success "$command: changed submodule worktree is reset" ' + prolog && + reset_work_tree_to_interested add_sub1 && + ( + cd submodule_update && + rm sub1/file1 && + : >sub1/new_file && + git -C sub1 add new_file && + $command HEAD && + test_path_is_file sub1/file1 && + test_path_is_missing sub1/new_file + ) + ' } -- 2.15.1.620.gb9897f4670-goog ^ permalink raw reply related [flat|nested] 38+ messages in thread
* Re: [PATCHv3 0/5] Fix --recurse-submodules for submodule worktree changes 2018-01-03 1:12 ` [PATCHv3 0/5] Fix --recurse-submodules for submodule worktree changes Stefan Beller ` (4 preceding siblings ...) 2018-01-03 1:12 ` [PATCH 5/5] submodule: submodule_move_head omits old argument in forced case Stefan Beller @ 2018-01-03 20:49 ` Junio C Hamano 2018-01-03 21:16 ` Stefan Beller 2018-01-05 20:03 ` [PATCHv4 0/4] " Stefan Beller 5 siblings, 2 replies; 38+ messages in thread From: Junio C Hamano @ 2018-01-03 20:49 UTC (permalink / raw) To: Stefan Beller; +Cc: git, jrnieder Stefan Beller <sbeller@google.com> writes: > Thanks Junio for review of this series! > The only change in this version of the series is > > --- a/unpack-trees.c > +++ b/unpack-trees.c > @@ -2140,7 +2140,7 @@ int oneway_merge(const struct cache_entry * const *src, > update |= CE_UPDATE; > } > if (S_ISGITLINK(old->ce_mode) && should_update_submodules() && > - !verify_uptodate(old, o)) > + o->update && !verify_uptodate(old, o)) > update |= CE_UPDATE; > add_entry(o, old, update, 0); > Sounds OK. I wonder why o->update is not at the very beginning of the &&-chain, though. After all, the one above this addition begins with o->reset && o->update *not* because of the performance concern, but primarily due to logic flow. I.e. "if we are resetting and updating the working tree, then..." comes first before saying "we may need to flip CE_UPDATE bit in update variable if the file in the working tree is not up to date and it is within a narrow checkout area". Of course, because verify_uptodate() is rather expensive, checking o->update before that makes sense from micro-optimization's point of view, too. So after thinking aloud like the above, I am reasonably sure that you want to check o->update as the very first thing in this new if statement. > v2: > I dropped the patch to `same()` as I realized we only need to fix the > oneway_merge function, the others (two, three way merge) are fine as > they have the checks already in place. This is a bit flawed argument, no? Checking working tree paths unconditionally in same(), which does not even know if we are touching the working tree paths, is broken. Unless "they have the checks already in place" refers to checks that bypasses calls to same() when we are not touching working tree paths, that is, but obviously that is not what is going on. Will queue. Thanks for working on this. ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCHv3 0/5] Fix --recurse-submodules for submodule worktree changes 2018-01-03 20:49 ` [PATCHv3 0/5] Fix --recurse-submodules for submodule worktree changes Junio C Hamano @ 2018-01-03 21:16 ` Stefan Beller 2018-01-05 20:03 ` [PATCHv4 0/4] " Stefan Beller 1 sibling, 0 replies; 38+ messages in thread From: Stefan Beller @ 2018-01-03 21:16 UTC (permalink / raw) To: Junio C Hamano; +Cc: git, Jonathan Nieder On Wed, Jan 3, 2018 at 12:49 PM, Junio C Hamano <gitster@pobox.com> wrote: > Stefan Beller <sbeller@google.com> writes: > >> Thanks Junio for review of this series! >> The only change in this version of the series is >> >> --- a/unpack-trees.c >> +++ b/unpack-trees.c >> @@ -2140,7 +2140,7 @@ int oneway_merge(const struct cache_entry * const *src, >> update |= CE_UPDATE; >> } >> if (S_ISGITLINK(old->ce_mode) && should_update_submodules() && >> - !verify_uptodate(old, o)) >> + o->update && !verify_uptodate(old, o)) >> update |= CE_UPDATE; >> add_entry(o, old, update, 0); >> > > Sounds OK. > > I wonder why o->update is not at the very beginning of the &&-chain, > though. After all, the one above this addition begins with o->reset > && o->update *not* because of the performance concern, but primarily > due to logic flow. I.e. "if we are resetting and updating the > working tree, then..." comes first before saying "we may need to > flip CE_UPDATE bit in update variable if the file in the working > tree is not up to date and it is within a narrow checkout area". It shows that I work too much with submodules. ;) "If we have a submodule and ..." seemed to be the important part when writing the patch. > Of course, because verify_uptodate() is rather expensive, checking > o->update before that makes sense from micro-optimization's point of > view, too. I would think S_ISGITLINK, should_update_submodules as well as o->update are all on the same order of magnitude of costs (some couple number of operations) when compared to verify_uptodate (spawning processes), so as long as verify_uptodate goes last we'd be fine. > > So after thinking aloud like the above, I am reasonably sure that > you want to check o->update as the very first thing in this new if > statement. Thanks for double checking and thinking about the code base with a less submodule centric point of view. Mind to squash it locally or want me to resend? For a resend I'll wait a couple of days to see if there are more comments needing to be addressed. > >> v2: >> I dropped the patch to `same()` as I realized we only need to fix the >> oneway_merge function, the others (two, three way merge) are fine as >> they have the checks already in place. > > This is a bit flawed argument, no? Checking working tree paths > unconditionally in same(), which does not even know if we are > touching the working tree paths, is broken. Unless "they have the > checks already in place" refers to checks that bypasses calls to > same() when we are not touching working tree paths, that is, but > obviously that is not what is going on. > > Will queue. Thanks for working on this. > > ^ permalink raw reply [flat|nested] 38+ messages in thread
* [PATCHv4 0/4] Fix --recurse-submodules for submodule worktree changes 2018-01-03 20:49 ` [PATCHv3 0/5] Fix --recurse-submodules for submodule worktree changes Junio C Hamano 2018-01-03 21:16 ` Stefan Beller @ 2018-01-05 20:03 ` Stefan Beller 2018-01-05 20:03 ` [PATCHv4 1/4] t/lib-submodule-update.sh: clarify test Stefan Beller ` (4 more replies) 1 sibling, 5 replies; 38+ messages in thread From: Stefan Beller @ 2018-01-05 20:03 UTC (permalink / raw) To: gitster; +Cc: git, Stefan Beller v4: * dropped the patch "t/helper/test-lazy-name-hash: fix compilation" (that snuck into v3 by accident, it is already present at remotes/origin/jh/memihash-opt, but I am carrying it locally in most branches currently.) * Junio pointed out the micro-optimisation below, both in terms of readability as well as performance diff --git a/unpack-trees.c b/unpack-trees.c index 7657d6ecdd..96c3327f19 100644 --- a/unpack-trees.c +++ b/unpack-trees.c @@ -2139,8 +2139,8 @@ int oneway_merge(const struct cache_entry * const *src, ie_match_stat(o->src_index, old, &st, CE_MATCH_IGNORE_VALID|CE_MATCH_IGNORE_SKIP_WORKTREE)) update |= CE_UPDATE; } - if (S_ISGITLINK(old->ce_mode) && should_update_submodules() && - o->update && !verify_uptodate(old, o)) + if (o->update && S_ISGITLINK(old->ce_mode) && + should_update_submodules() && !verify_uptodate(old, o)) update |= CE_UPDATE; add_entry(o, old, update, 0); return 0; v3: Thanks Junio for review of this series! The only change in this version of the series is --- a/unpack-trees.c +++ b/unpack-trees.c @@ -2140,7 +2140,7 @@ int oneway_merge(const struct cache_entry * const *src, update |= CE_UPDATE; } if (S_ISGITLINK(old->ce_mode) && should_update_submodules() && - !verify_uptodate(old, o)) + o->update && !verify_uptodate(old, o)) update |= CE_UPDATE; add_entry(o, old, update, 0); v2: I dropped the patch to `same()` as I realized we only need to fix the oneway_merge function, the others (two, three way merge) are fine as they have the checks already in place. The test added in the last patch got slightly larger as now we also test for newly staged files to be blown away in the submodule. v1: The fix is in the last patch, the first patches are just massaging the code base to make the fix easy. The second patch fixes a bug in the test, which was ineffective at testing. The third patch shows the problem this series addresses, the fourth patch is a little refactoring, which I want to keep separate as I would expect it to be a performance regression[1]. The first patch is unrelated, but improves the readability of submodule test cases, which we'd want to improve further. Thanks, Stefan Stefan Beller (4): t/lib-submodule-update.sh: clarify test t/lib-submodule-update.sh: Fix test ignoring ignored files in submodules unpack-trees: oneway_merge to update submodules submodule: submodule_move_head omits old argument in forced case submodule.c | 4 +++- t/lib-submodule-update.sh | 19 +++++++++++++++++-- unpack-trees.c | 3 +++ 3 files changed, 23 insertions(+), 3 deletions(-) -- 2.16.0.rc0.223.g4a4ac83678-goog ^ permalink raw reply related [flat|nested] 38+ messages in thread
* [PATCHv4 1/4] t/lib-submodule-update.sh: clarify test 2018-01-05 20:03 ` [PATCHv4 0/4] " Stefan Beller @ 2018-01-05 20:03 ` Stefan Beller 2018-01-05 20:03 ` [PATCHv4 2/4] t/lib-submodule-update.sh: Fix test ignoring ignored files in submodules Stefan Beller ` (3 subsequent siblings) 4 siblings, 0 replies; 38+ messages in thread From: Stefan Beller @ 2018-01-05 20:03 UTC (permalink / raw) To: gitster; +Cc: git, Stefan Beller Keep the local branch name as the upstream branch name to avoid confusion. Signed-off-by: Stefan Beller <sbeller@google.com> Signed-off-by: Junio C Hamano <gitster@pobox.com> --- t/lib-submodule-update.sh | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/t/lib-submodule-update.sh b/t/lib-submodule-update.sh index 38dadd2c29..d7699046f6 100755 --- a/t/lib-submodule-update.sh +++ b/t/lib-submodule-update.sh @@ -664,8 +664,8 @@ test_submodule_recursing_with_args_common() { cd submodule_update && git -C sub1 checkout -b keep_branch && git -C sub1 rev-parse HEAD >expect && - git branch -t check-keep origin/modify_sub1 && - $command check-keep && + git branch -t modify_sub1 origin/modify_sub1 && + $command modify_sub1 && test_superproject_content origin/modify_sub1 && test_submodule_content sub1 origin/modify_sub1 && git -C sub1 rev-parse keep_branch >actual && -- 2.16.0.rc0.223.g4a4ac83678-goog ^ permalink raw reply related [flat|nested] 38+ messages in thread
* [PATCHv4 2/4] t/lib-submodule-update.sh: Fix test ignoring ignored files in submodules 2018-01-05 20:03 ` [PATCHv4 0/4] " Stefan Beller 2018-01-05 20:03 ` [PATCHv4 1/4] t/lib-submodule-update.sh: clarify test Stefan Beller @ 2018-01-05 20:03 ` Stefan Beller 2018-01-05 20:03 ` [PATCHv4 3/4] unpack-trees: oneway_merge to update submodules Stefan Beller ` (2 subsequent siblings) 4 siblings, 0 replies; 38+ messages in thread From: Stefan Beller @ 2018-01-05 20:03 UTC (permalink / raw) To: gitster; +Cc: git, Stefan Beller It turns out that the test replacing a submodule with a file with the submodule containing an ignored file is incorrectly titled, because the test put the file in place, but never ignored that file. When having an untracked file Instead of an ignored file in the submodule, git should refuse to remove the submodule, but that is a bug in the implementation of recursing into submodules, such that the test just passed, removing the untracked file. Fix the test first; in a later patch we'll fix gits behavior, that will make sure untracked files are not deleted. Signed-off-by: Stefan Beller <sbeller@google.com> Signed-off-by: Junio C Hamano <gitster@pobox.com> --- t/lib-submodule-update.sh | 1 + 1 file changed, 1 insertion(+) diff --git a/t/lib-submodule-update.sh b/t/lib-submodule-update.sh index d7699046f6..fb0173ea87 100755 --- a/t/lib-submodule-update.sh +++ b/t/lib-submodule-update.sh @@ -885,6 +885,7 @@ test_submodule_switch_recursing_with_args () { ( cd submodule_update && git branch -t replace_sub1_with_file origin/replace_sub1_with_file && + echo ignored >.git/modules/sub1/info/exclude && : >sub1/ignored && $command replace_sub1_with_file && test_superproject_content origin/replace_sub1_with_file && -- 2.16.0.rc0.223.g4a4ac83678-goog ^ permalink raw reply related [flat|nested] 38+ messages in thread
* [PATCHv4 3/4] unpack-trees: oneway_merge to update submodules 2018-01-05 20:03 ` [PATCHv4 0/4] " Stefan Beller 2018-01-05 20:03 ` [PATCHv4 1/4] t/lib-submodule-update.sh: clarify test Stefan Beller 2018-01-05 20:03 ` [PATCHv4 2/4] t/lib-submodule-update.sh: Fix test ignoring ignored files in submodules Stefan Beller @ 2018-01-05 20:03 ` Stefan Beller 2018-01-05 20:03 ` [PATCHv4 4/4] submodule: submodule_move_head omits old argument in forced case Stefan Beller 2018-01-09 22:45 ` [PATCHv4 0/4] Fix --recurse-submodules for submodule worktree changes Junio C Hamano 4 siblings, 0 replies; 38+ messages in thread From: Stefan Beller @ 2018-01-05 20:03 UTC (permalink / raw) To: gitster; +Cc: git, Stefan Beller When there is a one way merge, each submodule needs to be one way merged as well, if we're asked to recurse into submodules. In case of a submodule, check if it is up-to-date, otherwise set the flag CE_UPDATE, which will trigger an update of it in the phase updating the tree later. Signed-off-by: Stefan Beller <sbeller@google.com> Signed-off-by: Junio C Hamano <gitster@pobox.com> --- unpack-trees.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/unpack-trees.c b/unpack-trees.c index bf8b602901..96c3327f19 100644 --- a/unpack-trees.c +++ b/unpack-trees.c @@ -2139,6 +2139,9 @@ int oneway_merge(const struct cache_entry * const *src, ie_match_stat(o->src_index, old, &st, CE_MATCH_IGNORE_VALID|CE_MATCH_IGNORE_SKIP_WORKTREE)) update |= CE_UPDATE; } + if (o->update && S_ISGITLINK(old->ce_mode) && + should_update_submodules() && !verify_uptodate(old, o)) + update |= CE_UPDATE; add_entry(o, old, update, 0); return 0; } -- 2.16.0.rc0.223.g4a4ac83678-goog ^ permalink raw reply related [flat|nested] 38+ messages in thread
* [PATCHv4 4/4] submodule: submodule_move_head omits old argument in forced case 2018-01-05 20:03 ` [PATCHv4 0/4] " Stefan Beller ` (2 preceding siblings ...) 2018-01-05 20:03 ` [PATCHv4 3/4] unpack-trees: oneway_merge to update submodules Stefan Beller @ 2018-01-05 20:03 ` Stefan Beller 2018-01-09 22:45 ` [PATCHv4 0/4] Fix --recurse-submodules for submodule worktree changes Junio C Hamano 4 siblings, 0 replies; 38+ messages in thread From: Stefan Beller @ 2018-01-05 20:03 UTC (permalink / raw) To: gitster; +Cc: git, Stefan Beller When using hard reset or forced checkout with the option to recurse into submodules, the submodules need to be reset, too. It turns out that we need to omit the duplicate old argument to read-tree in all forced cases to omit the 2 way merge and use the more assertive behavior of reading the specific new tree into the index and updating the working tree. Signed-off-by: Stefan Beller <sbeller@google.com> Signed-off-by: Junio C Hamano <gitster@pobox.com> --- submodule.c | 4 +++- t/lib-submodule-update.sh | 14 ++++++++++++++ 2 files changed, 17 insertions(+), 1 deletion(-) diff --git a/submodule.c b/submodule.c index 2967704317..47ddc9b273 100644 --- a/submodule.c +++ b/submodule.c @@ -1657,7 +1657,9 @@ int submodule_move_head(const char *path, else argv_array_push(&cp.args, "-m"); - argv_array_push(&cp.args, old ? old : EMPTY_TREE_SHA1_HEX); + if (!(flags & SUBMODULE_MOVE_HEAD_FORCE)) + argv_array_push(&cp.args, old ? old : EMPTY_TREE_SHA1_HEX); + argv_array_push(&cp.args, new ? new : EMPTY_TREE_SHA1_HEX); if (run_command(&cp)) { diff --git a/t/lib-submodule-update.sh b/t/lib-submodule-update.sh index fb0173ea87..1f38a85371 100755 --- a/t/lib-submodule-update.sh +++ b/t/lib-submodule-update.sh @@ -1015,4 +1015,18 @@ test_submodule_forced_switch_recursing_with_args () { test_submodule_content sub1 origin/modify_sub1 ) ' + + test_expect_success "$command: changed submodule worktree is reset" ' + prolog && + reset_work_tree_to_interested add_sub1 && + ( + cd submodule_update && + rm sub1/file1 && + : >sub1/new_file && + git -C sub1 add new_file && + $command HEAD && + test_path_is_file sub1/file1 && + test_path_is_missing sub1/new_file + ) + ' } -- 2.16.0.rc0.223.g4a4ac83678-goog ^ permalink raw reply related [flat|nested] 38+ messages in thread
* Re: [PATCHv4 0/4] Fix --recurse-submodules for submodule worktree changes 2018-01-05 20:03 ` [PATCHv4 0/4] " Stefan Beller ` (3 preceding siblings ...) 2018-01-05 20:03 ` [PATCHv4 4/4] submodule: submodule_move_head omits old argument in forced case Stefan Beller @ 2018-01-09 22:45 ` Junio C Hamano 4 siblings, 0 replies; 38+ messages in thread From: Junio C Hamano @ 2018-01-09 22:45 UTC (permalink / raw) To: Stefan Beller; +Cc: git Stefan Beller <sbeller@google.com> writes: > v4: > Stefan Beller (4): > t/lib-submodule-update.sh: clarify test > t/lib-submodule-update.sh: Fix test ignoring ignored files in > submodules > unpack-trees: oneway_merge to update submodules > submodule: submodule_move_head omits old argument in forced case > > submodule.c | 4 +++- > t/lib-submodule-update.sh | 19 +++++++++++++++++-- > unpack-trees.c | 3 +++ > 3 files changed, 23 insertions(+), 3 deletions(-) Thanks. This one looks excellent. Let's merge it to 'next' and cook it there. ^ permalink raw reply [flat|nested] 38+ messages in thread
* [PATCHv2 5/5] submodule: submodule_move_head omits old argument in forced case 2017-12-27 22:57 ` [PATCHv2 0/5] Fix --recurse-submodules for submodule worktree changes Stefan Beller ` (3 preceding siblings ...) 2017-12-27 22:57 ` [PATCHv2 4/5] unpack-trees: oneway_merge to update submodules Stefan Beller @ 2017-12-27 22:57 ` Stefan Beller 4 siblings, 0 replies; 38+ messages in thread From: Stefan Beller @ 2017-12-27 22:57 UTC (permalink / raw) To: sbeller; +Cc: git, jrnieder When using hard reset or forced checkout with the option to recurse into submodules, the submodules need to be reset, too. It turns out that we need to omit the duplicate old argument to read-tree in all forced cases to omit the 2 way merge and use the more assertive behavior of reading the specific new tree into the index and updating the working tree. Signed-off-by: Stefan Beller <sbeller@google.com> --- submodule.c | 4 +++- t/lib-submodule-update.sh | 14 ++++++++++++++ 2 files changed, 17 insertions(+), 1 deletion(-) diff --git a/submodule.c b/submodule.c index fa25888783..db0f7ac51e 100644 --- a/submodule.c +++ b/submodule.c @@ -1653,7 +1653,9 @@ int submodule_move_head(const char *path, else argv_array_push(&cp.args, "-m"); - argv_array_push(&cp.args, old ? old : EMPTY_TREE_SHA1_HEX); + if (!(flags & SUBMODULE_MOVE_HEAD_FORCE)) + argv_array_push(&cp.args, old ? old : EMPTY_TREE_SHA1_HEX); + argv_array_push(&cp.args, new ? new : EMPTY_TREE_SHA1_HEX); if (run_command(&cp)) { diff --git a/t/lib-submodule-update.sh b/t/lib-submodule-update.sh index fb0173ea87..1f38a85371 100755 --- a/t/lib-submodule-update.sh +++ b/t/lib-submodule-update.sh @@ -1015,4 +1015,18 @@ test_submodule_forced_switch_recursing_with_args () { test_submodule_content sub1 origin/modify_sub1 ) ' + + test_expect_success "$command: changed submodule worktree is reset" ' + prolog && + reset_work_tree_to_interested add_sub1 && + ( + cd submodule_update && + rm sub1/file1 && + : >sub1/new_file && + git -C sub1 add new_file && + $command HEAD && + test_path_is_file sub1/file1 && + test_path_is_missing sub1/new_file + ) + ' } -- 2.15.1.620.gb9897f4670-goog ^ permalink raw reply related [flat|nested] 38+ messages in thread
end of thread, other threads:[~2018-01-09 22:45 UTC | newest] Thread overview: 38+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-12-19 22:26 [PATCH 0/5] Fix --recurse-submodules for submodule worktree changes Stefan Beller 2017-12-19 22:26 ` [PATCH 1/5] t/lib-submodule-update.sh: clarify test Stefan Beller 2017-12-19 22:26 ` [PATCH 2/5] t/lib-submodule-update.sh: Fix test ignoring ignored files in submodules Stefan Beller 2017-12-22 19:34 ` Junio C Hamano 2017-12-27 19:27 ` Stefan Beller 2017-12-19 22:26 ` [PATCH 3/5] t/lib-submodule-update.sh: add new test for submodule internal change Stefan Beller 2017-12-19 22:31 ` Jonathan Nieder 2017-12-19 22:35 ` Stefan Beller 2017-12-19 22:26 ` [PATCH 4/5] unpack-trees: Fix same() for submodules Stefan Beller 2017-12-19 22:26 ` [PATCH 5/5] submodule: submodule_move_head omits old argument in forced case Stefan Beller 2017-12-19 22:44 ` Jonathan Nieder 2017-12-19 22:54 ` Stefan Beller 2017-12-19 23:15 ` Jonathan Nieder 2017-12-20 0:01 ` Jonathan Nieder 2017-12-20 19:36 ` Stefan Beller 2017-12-20 19:38 ` Stefan Beller 2017-12-27 22:57 ` [PATCHv2 0/5] Fix --recurse-submodules for submodule worktree changes Stefan Beller 2017-12-27 22:57 ` [PATCHv2 1/5] t/helper/test-lazy-name-hash: fix compilation Stefan Beller 2017-12-27 22:57 ` [PATCHv2 2/5] t/lib-submodule-update.sh: clarify test Stefan Beller 2017-12-27 22:57 ` [PATCHv2 3/5] t/lib-submodule-update.sh: Fix test ignoring ignored files in submodules Stefan Beller 2017-12-27 22:57 ` [PATCHv2 4/5] unpack-trees: oneway_merge to update submodules Stefan Beller 2018-01-02 19:43 ` Junio C Hamano 2018-01-02 23:04 ` Stefan Beller 2018-01-03 1:12 ` [PATCHv3 0/5] Fix --recurse-submodules for submodule worktree changes Stefan Beller 2018-01-03 1:12 ` [PATCH 1/5] t/helper/test-lazy-name-hash: fix compilation Stefan Beller 2018-01-03 1:12 ` [PATCH 2/5] t/lib-submodule-update.sh: clarify test Stefan Beller 2018-01-03 1:12 ` [PATCH 3/5] t/lib-submodule-update.sh: Fix test ignoring ignored files in submodules Stefan Beller 2018-01-03 1:12 ` [PATCH 4/5] unpack-trees: oneway_merge to update submodules Stefan Beller 2018-01-03 1:12 ` [PATCH 5/5] submodule: submodule_move_head omits old argument in forced case Stefan Beller 2018-01-03 20:49 ` [PATCHv3 0/5] Fix --recurse-submodules for submodule worktree changes Junio C Hamano 2018-01-03 21:16 ` Stefan Beller 2018-01-05 20:03 ` [PATCHv4 0/4] " Stefan Beller 2018-01-05 20:03 ` [PATCHv4 1/4] t/lib-submodule-update.sh: clarify test Stefan Beller 2018-01-05 20:03 ` [PATCHv4 2/4] t/lib-submodule-update.sh: Fix test ignoring ignored files in submodules Stefan Beller 2018-01-05 20:03 ` [PATCHv4 3/4] unpack-trees: oneway_merge to update submodules Stefan Beller 2018-01-05 20:03 ` [PATCHv4 4/4] submodule: submodule_move_head omits old argument in forced case Stefan Beller 2018-01-09 22:45 ` [PATCHv4 0/4] Fix --recurse-submodules for submodule worktree changes Junio C Hamano 2017-12-27 22:57 ` [PATCHv2 5/5] submodule: submodule_move_head omits old argument in forced case Stefan Beller
Code repositories for project(s) associated with this public inbox https://80x24.org/mirrors/git.git This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).