From: Yaroslav O Halchenko <debian@onerussian.com>
To: git <git@vger.kernel.org>
Subject: Re: [PATCH 2/2] RF+ENH(TST): compare the entire list of submodule status --recursive to stay intact
Date: Thu, 13 Dec 2018 11:42:17 -0500 [thread overview]
Message-ID: <20181213164217.GA4633@hopa.kiewit.dartmouth.edu> (raw)
In-Reply-To: <CAGZ79kY17gmEh5Sawa+1fG5cXjOReOgCjDyEmGbbpJ5EE1APdw@mail.gmail.com>
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?
--
Yaroslav O. Halchenko
Center for Open Neuroscience http://centerforopenneuroscience.org
Dartmouth College, 419 Moore Hall, Hinman Box 6207, Hanover, NH 03755
Phone: +1 (603) 646-9834 Fax: +1 (603) 646-1419
WWW: http://www.linkedin.com/in/yarik
next prev parent reply other threads:[~2018-12-13 16:42 UTC|newest]
Thread overview: 18+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-12-06 17:35 [wishlist] git submodule update --reset-hard Yaroslav Halchenko
2018-12-06 18:29 ` Stefan Beller
2018-12-06 21:24 ` Yaroslav Halchenko
2018-12-06 21:55 ` Stefan Beller
2018-12-07 1:22 ` Yaroslav Halchenko
2018-12-07 21:55 ` Stefan Beller
2018-12-08 2:15 ` Yaroslav Halchenko
2018-12-08 4:21 ` Yaroslav Halchenko
2018-12-10 18:58 ` Stefan Beller
2018-12-10 20:14 ` Yaroslav Halchenko
2018-12-11 4:08 ` [PATCH 1/2] submodule: Add --reset-hard option for git submodule update Yaroslav Halchenko
2018-12-11 4:08 ` [PATCH 2/2] RF+ENH(TST): compare the entire list of submodule status --recursive to stay intact Yaroslav Halchenko
2018-12-12 19:48 ` Stefan Beller
2018-12-13 16:42 ` Yaroslav O Halchenko [this message]
2018-12-13 20:44 ` Stefan Beller
2018-12-13 22:43 ` Yaroslav O Halchenko
2018-12-13 23:58 ` Stefan Beller
2018-12-14 4:22 ` Yaroslav O Halchenko
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20181213164217.GA4633@hopa.kiewit.dartmouth.edu \
--to=debian@onerussian.com \
--cc=git@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).