On Wed, Oct 10, 2018 at 11:56 AM Antonio Ospite <ao2@xxxxxx> wrote:
>
> On Mon, 8 Oct 2018 15:19:00 -0700
> Stefan Beller <sbeller@xxxxxxxxxx> wrote:
>
> > > +test_expect_success 'not writing gitmodules config file when it is not checked out' '
> > > + test_must_fail git -C super submodule--helper config submodule.submodule.url newurl
> >
> > This only checks the exit code, do we also want to check for
> >
> > test_path_is_missing .gitmodules ?
> >
>
> OK, I agree, let's re-check also *after* we tried and failed to set
> a config value, just to be sure that the code does not get accidentally
> changed in the future to create the file. I'll add the check.
>
> > > +test_expect_success 'initialising submodule when the gitmodules config is not checked out' '
> > > + git -C super submodule init
> > > +'
> > > +
> > > +test_expect_success 'showing submodule summary when the gitmodules config is not checked out' '
> > > + git -C super submodule summary
> > > +'
> >
> > Same for these, is the exit code enough, or do we want to look at
> > specific things?
> >
>
> Except for the "summary" test which was not even exercising the
> config_from_gitmodule path, checking exist status should be sufficient
> to verify that "submodule--helper config" does not fail, but we can
> surely do better.
>
> I will add checks to confirm that not only the commands exited without
> errors but they also achieved the desired effect, to validate the actual
> high-level use case advertised by the test file. This should be more
> future-proof.
>
> And I think I'll merge the summary and the update tests.
>
> > > +
> > > +test_expect_success 'updating submodule when the gitmodules config is not checked out' '
> > > + (cd submodule &&
> > > + echo file2 >file2 &&
> > > + git add file2 &&
> > > + git commit -m "add file2 to submodule"
> > > + ) &&
> > > + git -C super submodule update
> >
> > git status would want to be clean afterwards?
>
> Mmh, this should have been "submodule update --remote" in the first
> place to have any effect, I'll take the chance and rewrite this test in
> a different way and also check the effect of the update operation, and
> the repository status.
>
> I'll be something like this:
>
> ORIG_SUBMODULE=$(git -C submodule rev-parse HEAD)
> ORIG_UPSTREAM=$(git -C upstream rev-parse HEAD)
> ORIG_SUPER=$(git -C super rev-parse HEAD)
>
> test_expect_success 're-updating submodule when the gitmodules config is not checked out' '
> test_when_finished "git -C submodule reset --hard $ORIG_SUBMODULE;
> git -C upstream reset --hard $ORIG_UPSTREAM;
> git -C super reset --hard $ORIG_SUPER;
> git -C upstream submodule update --remote;
> git -C super pull;
> git -C super submodule update --remote" &&
> (cd submodule &&
> echo file2 >file2 &&
> git add file2 &&
> test_tick &&
> git commit -m "add file2 to submodule"
> ) &&
> (cd upstream &&
> git submodule update --remote &&
> git add submodule &&
> test_tick &&
> git commit -m "Update submodule"
> ) &&
> git -C super pull &&
> # The --for-status options reads the gitmdoules config
gitmodules
> git -C super submodule summary --for-status >actual &&
> cat >expect <<-\EOF &&
> * submodule 951c301...a939200 (1):
hardcoding hash values burdens the plan to migrate to another
hash function,
rev1=$(git -C submodule rev-parse --short HEAD^)
rev2=$(git -C submodule rev-parse --short HEAD)
and then use ${rev1}..${rev2} ?
> < add file2 to submodule
>
> EOF
> test_cmp expect actual &&
> # Test that the update actually succeeds
> test_path_is_missing super/submodule/file2 &&
> git -C super submodule update &&
> test_cmp submodule/file2 super/submodule/file2 &&
> git -C super status --short >output &&
> test_must_be_empty output
> '
>
> Maybe a little overkill?
Wow, very thorough! You might call it overkill, but now that you have it...
> The "upstream" repo will be added in test 1 to better clarify the roles
> of the involved repositories.
>
> The commit ids should be stable because of test_tick, shouldn't they?
Yes, but see
Documentation/technical/hash-function-transition.txt
that a couple people are working on. Let's be nice to them. :-)
Stefan
