Re: [PATCH] ref namespaces: tests

[Josh Triplett <josh@joshtriplett.org>]

On Thu, Jul 14, 2011 at 04:13:48PM -0700, Junio C Hamano wrote:
> Josh Triplett <josh@joshtriplett.org> writes:
> >  create mode 100755 t/t5502-fetch-push-namespaces.sh
> 
> Isn't 5502 used already?

Argh.  Yes; I put it right after t5501-fetch-push-alternates.sh without
noticing that 5502 already existed.  I'll fix it to use t5507.  Thanks
for catching that.

> > +test_expect_success setup '
> > +	test_tick &&
> > +	git init original &&
> > +	(
> > +		cd original &&
> > +		i=0 &&
> > +		while [ "$i" -lt 2 ]
> > +		do
> > +			echo "$i" > count &&
> 
> This is just style, but the test scripts prefer to spell these like this:
> 
> 	while test "$i" -lt 2
> 	do
>         	echo "$i" >count &&
>                 ...
> 
> to favor "test" over "[ ... ]", and omit SP between ">" redirection (or
> "<" for that matter) and the filename.

Will do.  I had done a quick grep-survey of the tests to check usage of
test versus [, and saw enough of both to assume it didn't matter, but it
hadn't occurred to me to check CodingGuidelines for shell scripts; I now
see that it has a section specifically on shell scripting.  I'll fix
this in the next version.

Actually, I plan to unroll this two-iteration loop in the next version,
so that I can capture the two object hashes I need for use later in the
script.

Out of curiosity, what's the rationale for the use of test rather than
'['?  Just uniformity, or does test have some particular advantage over
'['?

> > +		git remote add pushee-namespaced "ext::git --namespace=namespace %s ../pushee" &&
> 
> Nice ;-).

Thanks. :)

> > +test_expect_success 'pushing into a repository using a ref namespace' '
> > +	(
> > +		cd original &&
> > +		git push pushee-namespaced master &&
> > +		git ls-remote pushee-namespaced > actual &&
> > +		printf "dc65a2e0f299dcc7efddbbe01641a28ee84329ba\trefs/heads/master\n" > expected &&
> 
> Could you avoid hardcoding the exact object names here?  Your script knows
> what object should appear at refs/heads/master at "pushee-namespaced" (as
> you have pushed from the repository "original" you are in), so it may be
> something like:
> 
> 	printf "%s\trefs/heads/mater\n" $(git rev-parse master) >expect
> 
> Same comment applies for all the other hardcoded object names.

I can do that; since the same two object hashes recur throughout the
script, I'll record them in shell variables up at the top.

> > +test_expect_success 'mirroring a repository using a ref namespace' '
> > +	git clone --mirror pushee mirror &&
> > +	(
> > +		cd mirror &&
> > +		git for-each-ref refs/ > actual &&
> > +		printf "dc65a2e0f299dcc7efddbbe01641a28ee84329ba commit\trefs/namespaces/namespace/refs/heads/master\n" > expected &&
> > +		printf "fbdf4310c71b916568f04753f603fb24a0544227 commit\trefs/namespaces/namespace/refs/tags/0\n" >> expected &&
> > +		printf "dc65a2e0f299dcc7efddbbe01641a28ee84329ba commit\trefs/namespaces/namespace/refs/tags/1\n" >> expected &&
> > +		test_cmp expected actual
> > +	)
> > +'
> 
> I am not sure what you are trying to test. "pushee" is pretending to be a
> hosting site that uses the namespace feature to house refs pushed from
> original in refs/namespaces/namespace/ so it is expected to have these
> refs under there.  You didn't make any "git remote" configuration in
> either mirror nor pushee, so it is natural with or without the namespace
> feature that "git clone --mirror" would find them at the same place.
> 
> What hasn't been tested in the above is to see what actual refs pushee has
> with (cd pushee && git for-each-ref), and you could argue that this test
> is a proxy for that, but then you are assuming that "clone --mirror" is
> not broken, which means it would make debugging harder when this test does
> start failing---is it the basic namespace feature, or is it mirror cloning
> that acquired a bug to break this test?

I wrote this test specifically to check for possible regressions in
clone or the machinery underneath it.  I wanted to ensure that no future
change caused clone to ignore refs in refs/namespaces/*.  In particular,
I want to protect against a regression caused by any future change to
the refs machinery that might cause it to ignore refs outside of
refs/heads/* or refs/tags/*, which might otherwise go un-noticed (as
they almost did during the development of this patchset, if not for an
incidental side effect of t5501).

If this test failed, I would expect that it would fail because clone
--mirror produced a mirrored repository which didn't actually contain
any refs, even though pushee contained the correctly namespaced refs;
thus, for-each-ref doesn't seem like the right test.

More generally, I also added this test because it tests a specific
high-level feature I care about: the ability to mirror a repository
containing namespaces using clone --mirror, and preserve those
namespaces.  I plan to use that as a backup mechanism, and I want it to
continue working. :)

- Josh Triplett