git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
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        

  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).