Re: [RFC PATCH v5] Implement --first-parent for git rev-list --bisect

[Johannes Schindelin <Johannes.Schindelin@gmx.de>]

Hi Junio,

On Fri, 6 Jul 2018, Junio C Hamano wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> >> > git rev-list --first-parent --bisect-all F..E >revs &&
> >> > test_line_count = 9 revs &&
> >> > for rev in E e1 e2 e3 e4 e5 e6 e7 e8
> >> > do
> >> >   grep "^$(git rev-parse $rev) " revs ||
> >> >   {
> >> >     echo "$rev not shown" >&2 &&
> >> >     return 1
> >> >   }
> >> > done &&
> >> > sed -e "s/.*(dist=\([0-9]*\)).*/\1/" revs >actual.dists &&
> >> > sort -r actual.dists >actual.dists.sorted &&
> >> > test_cmp actual.dists.sorted actual.dists
> >> 
> > From my point of view, this indicates that you want to set those exact
> > dist values in stone.
> 
> As I already said, I do not think it is absolutely necessary to
> declare that the minimum dist is 0 or 1, or how big one step of dist
> is.  For those reading from the sidelines, the history we are
> testing this new feature over looks like this
> 
> #     E		dist=0
> #    / \
> #   e1  |	dist=1
> #   |   |
> #   e2  |	dist=2
> #   |   |       ...
> #   |   |       ...
> #   e7  |	dist=2
> #   |   |
> #   e8  |	dist=1
> #    \ /
> #     F		dist=0
> 
> Current code will say dist=0 for E and F, dist=1 for e1 and e8,
> etc., and I am fine if the code suddenly start saying that E and F
> (i.e. those at the boundary of the graph) have dist=1 and one hop
> weighs 10 so dist=11 for e1 and e8 (i.e. those at one hop from the
> boundary).
> 
> But I am not fine if E and F get larger dist than e1 and e8, or e1
> and e8 get different ones.  I do not think the code quoted upfront
> would catch such future breakages.
> 
> And I also do not see a reason why somebody wants to make the dist
> computation to be 1-based (iow, changing the minimum from 0 to 1) or
> one step not to be 1 (iow, giving 11 to e1 and e8), so while I agree
> it is not strictly necessary to cast the concrete distance value in
> stone, I do not see much harm doing so *if* it helps to make it
> simpler the test that is necessary to make sure relative dist values
> assigned to these commits are in correct order.

I guess that you still want to misunderstand me.

Because it is not really hard to understand what I said: a good regression
test will verify precisely what you want to ensure, not precisely what the
command's output is.

So in this case, quite obviously what you want to do is to verify that E
and F get larger dist than e1 and e8. So that is what you test for. Not
some fixed text that might require adjusting in the future for any other
reason than a real bug.

Ciao,
Dscho