Re: [PATCH 2/2] RF+ENH(TST): compare the entire list of submodule status --recursive to stay intact

From: Stefan Beller <sbeller@google.com>
Date: Dec 12, 2018, 19:48 UTC
Message-ID: <CAGZ79kY17gmEh5Sawa+1fG5cXjOReOgCjDyEmGbbpJ5EE1APdw@mail.gmail.com>
In-Reply-To: <20181211040839.17472-2-debian@onerussian.com>

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

Show 5 quoted lines

> 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?

Show 10 quoted lines

>         ) &&
>         (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?