| Message ID | 20181211040839.17472-2-debian@onerussian.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/2] submodule: Add --reset-hard option for git submodule update | expand |
On Mon, Dec 10, 2018 at 8:09 PM Yaroslav Halchenko <debian@onerussian.com> wrote: Thanks for the patches. The first patch looks good to me! > [PATCH 2/2] RF+ENH(TST): compare the entire list of submodule status --recursive to stay intact The subject is a bit cryptic (specifically the first part before the colon), maybe t7406: compare entire submodule status for --reset-hard mode ? > For submodule update --reset-hard the best test is comparison of the > entire status as shown by submodule status --recursive. Upon update > --reset-hard we should get back to the original state, with all the > branches being the same (no detached HEAD) and commits identical to > original (so no merges, new commits, etc). "original state" can mean different things to different people. I'd think we could be more precise: ... we should get to the state that the submodule is reset to the object id as the superprojects gitlink points at, irrespective of the submodule branch. > test_expect_success 'submodule update --merge staying on master' ' > (cd super/submodule && > - git reset --hard HEAD~1 > + git reset --hard HEAD~1 unrelated white space change? > ) && > (cd super && > (cd submodule && > @@ -307,16 +318,28 @@ test_expect_success 'submodule update --merge staying on master' ' > ' > > test_expect_success 'submodule update --reset-hard staying on master' ' > [..] > +' > + The tests look good to me, though I wonder if we'd rather want to inline {record/compare}_submodule_status as then you'd not need to look it up and the functions are rather short?
Thank you Stefan for the review and please pardon my delay with the reply, and sorry it got a bit too long by the end ;) On Wed, 12 Dec 2018, Stefan Beller wrote: > Thanks for the patches. The first patch looks good to me! Great! > > [PATCH 2/2] RF+ENH(TST): compare the entire list of submodule status --recursive to stay intact > The subject is a bit cryptic (specifically the first part before the > colon), maybe > t7406: compare entire submodule status for --reset-hard mode > ? > > For submodule update --reset-hard the best test is comparison of the > > entire status as shown by submodule status --recursive. Upon update > > --reset-hard we should get back to the original state, with all the > > branches being the same (no detached HEAD) and commits identical to > > original (so no merges, new commits, etc). > "original state" can mean different things to different people. I'd think > we could be more precise: > ... we should get to the state that the submodule is reset to the > object id as the superprojects gitlink points at, irrespective of the > submodule branch. ok, I will update the description. But I wonder if there could be some short term to be used to describe the composite "git submodule status" and "git status" (refers to below ;)). > > test_expect_success 'submodule update --merge staying on master' ' > > (cd super/submodule && > > - git reset --hard HEAD~1 > > + git reset --hard HEAD~1 > unrelated white space change? I was tuning formatting to be uniform and I guess missed that this is in the other (not my) test. I will revert that piece, thanks! BTW -- should I just squash to PATCHes now? I kept them separate primarily to show the use of those helpers: > > ) && > > (cd super && > > (cd submodule && > > @@ -307,16 +318,28 @@ test_expect_success 'submodule update --merge staying on master' ' > > ' > > test_expect_success 'submodule update --reset-hard staying on master' ' > > [..] > > +' > > + > The tests look good to me, though I wonder if we'd rather want to inline > {record/compare}_submodule_status as then you'd not need to look it up > and the functions are rather short? compare_submodules_status is already a compound action, so code would become quite more "loaded" if it is expanded, e.g. instead of (cd super && record_submodules_status && (cd submodule && git reset --hard HEAD~1 ) && ! compare_submodules_status && git submodule update --reset-hard submodule && compare_submodules_status ) it would become something like this I guess? (cd super && git submodule status --recursive >expect && (cd submodule && git reset --hard HEAD~1 ) && ! {git submodule status --recursive >actual && test_i18ncmp expect actual;} && git submodule update --reset-hard submodule && {git submodule status --recursive >actual && test_i18ncmp expect actual;} ) IMHO a bit mouth full. I was thinking also to extend compare_ with additional testing e.g. using "git status" since "git submodule status" does not care about untracked files etc. For --reset-hard I would like to assure that it is not just some kind of a mixed reset leaving files behind. That would make tests even more overloaded. On that point: Although I also like explicit calls at times, I also do like test fixtures as a concept to do more testing around the actual test-specific code block, thus minimizing boiler plate, which even if explicit makes code actually harder to grasp (at least to me). Since for the majority of the --reset-hard tests the fixture and test(s) are pretty much the same, actually ideally I would have liked to have something like this: test_expect_unchanged_submodule_status 'submodule update --reset-hard staying on master' \ super \ '(cd submodule && git reset --hard HEAD~1)' \ 'git submodule update --reset-hard submodule' where I just pass the path to work in, the test setup function, and the test action. The rest (initial cd, record, run setup, verify that there is a change, run action, verify there is no changes) is done by the test_expect_unchanged_submodule_status in a uniform way, absorbing all the boiler plate. (I am not married to the name, could be more descriptive/generic may be) Then we could breed a good number of tests with little to no boiler plate, with only relevant pieces and as extended as needed testing done by this test_expect_unchanged_submodule_status helper. e.g smth like test_expect_unchanged_submodule_status 'submodule update --reset-hard staying on master when I do a new commit' \ super \ '(cd submodule && git commit --allow-empty -m "new one"' \ 'git submodule update --reset-hard submodule' and kaboom -- we have a new test. If we decide to test more -- just tune up test_expect_unchanged_submodule_status and done -- all the tests remain sufficiently prescribed. What do you think?
On Thu, Dec 13, 2018 at 8:42 AM Yaroslav O Halchenko <debian@onerussian.com> wrote: > > Thank you Stefan for the review and please pardon my delay with the > reply, and sorry it got a bit too long by the end ;) > > On Wed, 12 Dec 2018, Stefan Beller wrote: > > Thanks for the patches. The first patch looks good to me! > > Great! > > > > [PATCH 2/2] RF+ENH(TST): compare the entire list of submodule status --recursive to stay intact > > > The subject is a bit cryptic (specifically the first part before the > > colon), maybe > > > t7406: compare entire submodule status for --reset-hard mode > > > ? > > > > > For submodule update --reset-hard the best test is comparison of the > > > entire status as shown by submodule status --recursive. Upon update > > > --reset-hard we should get back to the original state, with all the > > > branches being the same (no detached HEAD) and commits identical to > > > original (so no merges, new commits, etc). > > > "original state" can mean different things to different people. I'd think > > we could be more precise: > > > ... we should get to the state that the submodule is reset to the > > object id as the superprojects gitlink points at, irrespective of the > > submodule branch. > > ok, I will update the description. But I wonder if there could be some > short term to be used to describe the composite "git submodule status" > and "git status" (refers to below ;)). > > > > test_expect_success 'submodule update --merge staying on master' ' > > > (cd super/submodule && > > > - git reset --hard HEAD~1 > > > + git reset --hard HEAD~1 > > > unrelated white space change? > > I was tuning formatting to be uniform and I guess missed that this is in > the other (not my) test. I will revert that piece, thanks! The tests in that file are not quite following the coding style that is currently deemed the best. So if you want to clean that up as a preparatory patch, feel welcome to do so. :-) (c.f. t/t7400-submodule-basic.sh for good style, specifically indentation by tabs and the cd <path> on its own line in a subshell) The latest style update I found is 80938c39e2 (pack-objects test: modernize style, 2018-10-30) and submodule related test style 31158c7efc (t7410: update to new style, 2018-08-15) So I was not opposed to have style changes, but to have multiple unrelated things in one patch (feature work vs cleanup). > BTW -- should I just squash to PATCHes now? I kept them separate primarily to > show the use of those helpers: That would make sense. > compare_submodules_status is already a compound action, so code would > become quite more "loaded" if it is expanded, e.g. instead of ... > it would become something like this I guess? ... > ! {git submodule status --recursive >actual && you could keep the status out of the negation. > test_i18ncmp expect actual;} && > git submodule update --reset-hard submodule && > {git submodule status --recursive >actual && > test_i18ncmp expect actual;} > ) > > IMHO a bit mouth full. I was thinking also to extend compare_ with additional > testing e.g. using "git status" since "git submodule status" does not care > about untracked files etc. For --reset-hard I would like to assure that it is > not just some kind of a mixed reset leaving files behind. That would make > tests even more overloaded. ok, that makes sense. > On that point: Although I also like explicit calls at times, I also do > like test fixtures as a concept to do more testing around the actual > test-specific code block, thus minimizing boiler plate, which even if explicit > makes code actually harder to grasp (at least to me). > > Since for the majority of the --reset-hard tests the fixture and test(s) are > pretty much the same, actually ideally I would have liked to have > something like this: > > test_expect_unchanged_submodule_status 'submodule update --reset-hard staying on master' \ > super \ > '(cd submodule && git reset --hard HEAD~1)' \ > 'git submodule update --reset-hard submodule' > > where I just pass > the path to work in, > the test setup function, > and the test action. > > The rest (initial cd, record, run setup, verify that there is a change, run > action, verify there is no changes) is done by the > test_expect_unchanged_submodule_status in a uniform way, absorbing all the > boiler plate. (I am not married to the name, could be more descriptive/generic > may be) The issue with submodules is that we're already deviating from the 'standard' git test suite at times (See the submodule test suite lib-submodule-update.sh that is used via t1013, t2013 or t3906 and others). I guess if we keep the test_expect_unchanged_submodule_status as a file local function, it could be okay. > Then we could breed a good number of tests with little to no boiler plate, with > only relevant pieces and as extended as needed testing done by this > test_expect_unchanged_submodule_status helper. e.g smth like > > test_expect_unchanged_submodule_status 'submodule update --reset-hard staying on master when I do a new commit' \ > super \ > '(cd submodule && git commit --allow-empty -m "new one"' \ In new tests we're a big fan of using -C, as that can save the subshell, i.e. replace the whole line by git -C submodule commit --allow-empty -m "new one"' && > 'git submodule update --reset-hard submodule' > > and kaboom -- we have a new test. If we decide to test more -- just tune up > test_expect_unchanged_submodule_status and done -- all the tests remain > sufficiently prescribed. > > What do you think? That is pretty cool. Maybe my gut reaction on the previous patch also had to do with the numbers, i.e. having 2 extra function for only having 2 tests more legible. A framework is definitely better once we have more tests. Stefan
On Thu, 13 Dec 2018, Stefan Beller wrote: > > and kaboom -- we have a new test. If we decide to test more -- just tune up > > test_expect_unchanged_submodule_status and done -- all the tests remain > > sufficiently prescribed. > > What do you think? > That is pretty cool. Maybe my gut reaction on the previous patch > also had to do with the numbers, i.e. having 2 extra function for > only having 2 tests more legible. A framework is definitely better > once we have more tests. cool, thanks for the feedback - I will then try to make it happen quick one (so when I get to it I know): should I replicate all those tests you have for other update strategies? (treating of config specifications etc) There is no easy way to parametrize them somehow? ;) In Python world I might have mocked the actual underlying call to update, to see what option it would be getting and assure that it is the one I specified via config, and then sweepped through all of them to make sure nothing interim changes it. Just wondering if may be something like that exists in git's tests support. BTW - sorry if RTFM and unrelated, is there a way to update --merge but allowing only fast-forwards? My use case is collection of this submodules: http://datasets.datalad.org/?dir=/openneuro which all should come from github and I should not have any changes of my own. Sure thing if all is clean etc, merge should result in fast-forward. I just do not want to miss a case where there was some (temporary?) "dirt" which I forgot to reset and it would then get merged etc.
On Thu, Dec 13, 2018 at 2:44 PM Yaroslav O Halchenko <debian@onerussian.com> wrote: > > > On Thu, 13 Dec 2018, Stefan Beller wrote: > > > > and kaboom -- we have a new test. If we decide to test more -- just tune up > > > test_expect_unchanged_submodule_status and done -- all the tests remain > > > sufficiently prescribed. > > > > What do you think? > > > That is pretty cool. Maybe my gut reaction on the previous patch > > also had to do with the numbers, i.e. having 2 extra function for > > only having 2 tests more legible. A framework is definitely better > > once we have more tests. > > cool, thanks for the feedback - I will then try to make it happen > > quick one (so when I get to it I know): should I replicate all those > tests you have for other update strategies? (treating of config > specifications etc) If there is a sensible way to do so? I have the impression that there are enough differences, that it may not be possible to replicate all tests meaningfully from the other modes. > There is no easy way to parametrize them somehow? There is t/lib-submodule-update.sh, which brings this to an extreme, as it makes a "test suite in a test suite"; and I would not follow that example for this change. > ;) In Python world I might have mocked the actual underlying call to > update, to see what option it would be getting and assure that it is the > one I specified via config, and then sweepped through all of them > to make sure nothing interim changes it. Just wondering if may be > something like that exists in git's tests support. gits tests are very heavy on end to end testing, i.e. run a whole command and observe its output. This makes our command setup code, (i.e. finding the repository, parsing options, reading possible config, etc) a really well exercised code path. ;-) There is a recent push towards testing only units, most of t/helper is used for that, e.g. c.f. 4c7bb45269 (test-reach: test get_reachable_subset, 2018-11-02). So if you have a good idea how to focus the submodule tests more on the (new) unit that you add, that would be cool. > BTW - sorry if RTFM and unrelated, is there a way to > > update --merge > > but allowing only fast-forwards? My use case is collection of this > submodules: http://datasets.datalad.org/?dir=/openneuro which all > should come from github and I should not have any changes of my own. So you want the merge option --ff-only to be passed to the submodule merge command. I guess you could make a patch, that update takes another option (--ff-only, only useful when --merge is given), which is then propagated. I am not sure if we could have a more generalized option passing, which would allow to pass any option (for its respective command) to the command that is run in the update mode. > Sure thing if all is clean etc, merge should result in fast-forward. I > just do not want to miss a case where there was some (temporary?) "dirt" > which I forgot to reset and it would then get merged etc. maybe use --rebase, such that your potential change would bubble up and possibly produce a merge conflict?
On Thu, 13 Dec 2018, Stefan Beller wrote: > > cool, thanks for the feedback - I will then try to make it happen > > quick one (so when I get to it I know): should I replicate all those > > tests you have for other update strategies? (treating of config > > specifications etc) > If there is a sensible way to do so? > I have the impression that there are enough differences, that it > may not be possible to replicate all tests meaningfully from the > other modes. oh, by replicate I just meant to copy/paste and adjust for expected for --reset-hard test behavior (and possibly introduced helper), nothing fancy, just duplication as for replication ;-) > > There is no easy way to parametrize them somehow? > There is t/lib-submodule-update.sh, which brings this to > an extreme, as it makes a "test suite in a test suite"; and I would > not follow that example for this change. ok > > ;) In Python world I might have mocked the actual underlying call to > > update, to see what option it would be getting and assure that it is the > > one I specified via config, and then sweepped through all of them > > to make sure nothing interim changes it. Just wondering if may be > > something like that exists in git's tests support. > gits tests are very heavy on end to end testing, i.e. run a whole command > and observe its output. This makes our command setup code, (i.e. finding > the repository, parsing options, reading possible config, etc) a really well > exercised code path. ;-) > There is a recent push towards testing only units, most of > t/helper is used for that, e.g. c.f. 4c7bb45269 (test-reach: > test get_reachable_subset, 2018-11-02). > So if you have a good idea how to focus the submodule > tests more on the (new) unit that you add, that would be cool. no, not really any good ideas -- I am new here, but I will keep an eye open. > > BTW - sorry if RTFM and unrelated, is there a way to > > update --merge > > but allowing only fast-forwards? My use case is collection of this > > submodules: http://datasets.datalad.org/?dir=/openneuro which all > > should come from github and I should not have any changes of my own. > So you want the merge option --ff-only > to be passed to the submodule merge command. I guess you could make > a patch, that update takes another option (--ff-only, only useful when > --merge is given), which is then propagated. > I am not sure if we could have a more generalized option passing, > which would allow to pass any option (for its respective command) > to the command that is run in the update mode. wouldn't it be (theoretically) possible, in principle, to pass them via some config variable? e.g. instead of submodule update --reset-hard have -c submodule.update.reset.opts=--hard update --reset and then analogously -c submodule.update.merge.opts=--ff-only update --merge (--ff-only I guess would make no sense for any "supermodule" - a repo with submodules) > > Sure thing if all is clean etc, merge should result in fast-forward. I > > just do not want to miss a case where there was some (temporary?) "dirt" > > which I forgot to reset and it would then get merged etc. > maybe use --rebase, such that your potential change would bubble > up and possibly produce a merge conflict? that is a good idea as a workaround, thanks!
diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh index 2e08e0047c..1927424f47 100755 --- a/t/t7406-submodule-update.sh +++ b/t/t7406-submodule-update.sh @@ -21,6 +21,17 @@ compare_head() test "$sha_master" = "$sha_head" } +record_submodules_status() +{ + git submodule status --recursive >expect +} + +compare_submodules_status() +{ + git submodule status --recursive >actual && + test_i18ncmp expect actual +} + test_expect_success 'setup a submodule tree' ' echo file > file && @@ -294,7 +305,7 @@ test_expect_success 'submodule update --rebase staying on master' ' test_expect_success 'submodule update --merge staying on master' ' (cd super/submodule && - git reset --hard HEAD~1 + git reset --hard HEAD~1 ) && (cd super && (cd submodule && @@ -307,16 +318,28 @@ test_expect_success 'submodule update --merge staying on master' ' ' test_expect_success 'submodule update --reset-hard staying on master' ' - (cd super/submodule && - git reset --hard HEAD~1 - ) && (cd super && + record_submodules_status && (cd submodule && - compare_head + git reset --hard HEAD~1 ) && + ! compare_submodules_status && git submodule update --reset-hard submodule && - cd submodule && - compare_head + compare_submodules_status + ) +' + +test_expect_success 'submodule update --reset-hard in nested submodule' ' + (cd recursivesuper && + git submodule update --init --recursive && + record_submodules_status && + (cd super/submodule && + echo 123 >> file && + git commit -m "new commit" file + ) && + ! compare_submodules_status && + git submodule update --reset-hard --recursive && + compare_submodules_status ) '
For submodule update --reset-hard the best test is comparison of the entire status as shown by submodule status --recursive. Upon update --reset-hard we should get back to the original state, with all the branches being the same (no detached HEAD) and commits identical to original (so no merges, new commits, etc). For that, I have introduced two helpers: {record,compare}_submodules_status and an additional test for --reset-hard in nested submodule. I have kept this as a separate PATCH to demonstrate the diff from the original test composition as introduced in the prior patch, and this one where all tests could be of the same type: record_submodule_status && perform evil actions && ! compare_submodule_status && # to double check that evil was done git submodule --reset-hard . && compare_submodule_status # assure that we are all good Signed-off-by: Yaroslav Halchenko <debian@onerussian.com> --- t/t7406-submodule-update.sh | 37 ++++++++++++++++++++++++++++++------- 1 file changed, 30 insertions(+), 7 deletions(-)