Re: [PATCH 2/6] t1414: document some reflog-walk oddities

From: Jeff King <peff@peff.net>
Date: Jul 5, 2017, 21:27 UTC
Message-ID: <20170705212757.3ygxiasbi2e7v27p@sigill.intra.peff.net>
In-Reply-To: <xmqqh8yqix7p.fsf@gitster.mtv.corp.google.com>

On Wed, Jul 05, 2017 at 10:56:42AM -0700, Junio C Hamano wrote:

Show 5 quoted lines

> >   1. The fake parents are used for commit selection and
> >      display. So for example, "--merges" or "--no-merges"
> >      are useful, because the history appears as a linear
> 
> s/useful/useless/ perhaps?

Oops, yes ("not useful" is probably what I was going for).

Show 18 quoted lines

> > +test_expect_success 'set up some reflog entries' '
> > +	test_commit one &&
> > +	test_commit two &&
> > +	git checkout -b side HEAD^ &&
> > +	test_commit three &&
> > +	git merge --no-commit master &&
> > +	echo evil-merge-content >>one.t &&
> > +	test_tick &&
> > +	git commit --no-edit -a
> > +'
> 
> Mental note: logically, what we want to see in the log are:
> 
>     master:  one-->two
>     side:                one-->three-->(evil)
>     HEAD:    one-->two-->one-->three-->(evil)
> 
> where the middle one in HEAD is "switching from master to side".

Yeah. I was tempted to document that, but I think the expect.all mostly does that for HEAD (and I don't really check the others). The grossest thing is this numeric selection:

> > +test_expect_failure 'pathspec limiting handles merges' '
> > +	sed -n "1p;3p;5p" expect.all >expect &&

I tried to think of an easy way to pick them out by name but couldn't come up with one. I don't know if that sed invocation deserves a comment or not.

Show 6 quoted lines

> > +	do_walk -- one.t >actual &&
> > +	test_cmp expect actual
> > +'
> 
> OK (it was a bit tricky to see why the topmost one should, but the
> evilness of the merge makes it eligible).

Yeah, that was why I added the evilness. A more real-world test would perhaps be a file with an actual conflict that got resolved (but not matching either parent exactly). I actually started by adding the evil content to "three", which is a bit closer. But it passes even without the patch, because the diff to the first parent still matches.

So I dunno. I think it's OK as is. My main goal was just to make sure that my TREESAME hackery catches both normal and merge commits.

Show 16 quoted lines

> > +test_expect_failure 'walking multiple reflogs shows both' '
> > +	{
> > +		do_walk HEAD &&
> > +		do_walk side
> > +	} >expect &&
> > +	do_walk HEAD side >actual &&
> > +	test_cmp expect actual
> > +'
> 
> I somehow find this one a bit iffy.  
> 
> The order that commits appear in the "walk from HEAD and side at the
> same time" may want to be different from what this test expects.
> "rev-list --since=3.days -g master next", for example, would want to
> refrain from reading the reflog of 'master' for all 90 days before
> switching to the reflog of 'next', no?

I did make the ordering intentional. We process each reflog sequentially in turn. I don't think it would be wrong to interleave them by date, but I actually think it's useful for somebody who switches that behavior to consciously update the test. Maybe it's worth calling out in the commit message.

I stopped short of actually documenting and guaranteeing that behavior to the user. I don't know how comfortable we would be changing it later.

(As an aside, I also prowled through the documentation looking for any guarantees we make to the user about the fake-parent thing, but I couldn't find any. So I considered its user-visible effects an unwanted side effect of the implementation).

-Peff