From: Derrick Stolee <stolee@gmail.com>
Date: Wed, 18 Jul 2018 16:14:44 GMT
Subject: Re: [PATCH 08/16] test-reach: create new test tool for ref_newer
Message-ID: <043a5a75-6181-6a4c-e81f-9d82645d8f1b@gmail.com>
In-Reply-To: <20180716230019.257742-1-jonathantanmy@google.com>

On 7/16/2018 7:00 PM, Jonathan Tan wrote:
>> To use the new test-tool, use 'test-tool reach <method>' and provide
>> input to stdin that describes the inputs to the method. Currently, we
>> only implement the ref_newer method, which requires two commits. Use
>> lines "A:<committish>" and "B:<committish>" for the two inputs. We will
>> expand this input later to accommodate methods that take lists of
>> commits.
> It would be nice if "A" and "B" were "ancestor" and "descendant" (or
> something like that) instead, so that I don't have to check which
> direction the reach is calculated in.

Different methods will use different combinations. I do notice that I 
forgot to list the method parameters as part of the test-tool output. It 
should print "ref_newer(A,B)" so you know which input goes in which place.

>
>> +int cmd__reach(int ac, const char **av)
>> +{
>> +	struct object_id oid_A, oid_B;
>> +	struct strbuf buf = STRBUF_INIT;
>> +	struct repository *r = the_repository;
>> +
>> +	setup_git_directory();
>> +
>> +	if (ac < 2)
>> +		exit(1);
>> +
>> +
>> +	while (strbuf_getline(&buf, stdin) != EOF) {
>> +		struct object_id oid;
>> +		struct object *o;
>> +		struct commit *c;
>> +		if (buf.len < 3)
>> +			continue;
>> +
>> +		if (get_oid_committish(buf.buf + 2, &oid))
>> +			die("failed to resolve %s", buf.buf + 2);
> You can also use skip_prefix() instead of using arithmetic to determine
> the start of the OID.
>
>> +# Construct a grid-like commit graph with points (x,y)
>> +# with 1 <= x <= 10, 1 <= y <= 10, where (x,y) has
>> +# parents (x-1, y) and (x, y-1), keeping in mind that
>> +# we drop a parent if a coordinate is nonpositive.
>> +#
>> +#             (10,10)
>> +#            /       \
>> +#         (10,9)    (9,10)
>> +#        /     \   /      \
>> +#    (10,8)    (9,9)      (8,10)
>> +#   /     \    /   \      /    \
>> +#         ( continued...)
>> +#   \     /    \   /      \    /
>> +#    (3,1)     (2,2)      (1,3)
>> +#        \     /    \     /
>> +#         (2,1)      (2,1)
>> +#              \    /
>> +#              (1,1)
> This is quite a good design, thanks.
>
>> +# We use branch 'comit-x-y' to refer to (x,y).
> s/comit/commit/
>
>> +	git show-ref -s commit-7-7 | git commit-graph write --stdin-commits &&
>> +	mv .git/objects/info/commit-graph commit-graph-half &&
> My understanding is that this writes for 7-7 and all its ancestors,
> but...
>
>> +test_expect_success 'ref_newer:hit' '
>> +	cat >input <<-\EOF &&
>> +		A:commit-5-7
>> +		B:commit-2-3
>> +	EOF
>> +	printf "ref_newer:1\n" >expect &&
>> +	test_three_modes ref_newer
>> +'
>> +
>> +test_done
> ...both 5-7 and 2-3 are ancestors of 7-7, right? Which means that you
> don't test the "half" commit graph here. (It's probably sufficient to
> just adjust the numbers.)

Good point! Thanks. I'll just write the commit-graph starting at 
commit-5-5 and that should satisfy the point of the "mixed-mode" tests.

-Stolee