Re: [RFH] revision limiting sometimes ignored

From: Junio C Hamano <gitster@pobox.com>
Date: Feb 5, 2008, 23:44 UTC
Message-ID: <7vprvb6k9u.fsf@gitster.siamese.dyndns.org>
In-Reply-To: <alpine.LFD.1.00.0802051300050.3110@woody.linux-foundation.org>

Linus Torvalds <torvalds@linux-foundation.org> writes:

Show 5 quoted lines

> I really wonder if the right thing is not simply to admit that we consider 
> the commit time meaningful (within some fudge factor!), and then do:
>
>  - make commit warn if any parent commit date is in the future from the 
>    current commit date (allow a *small* fudge factor here, say 5 minutes).

Hmmm. In other words, you are punished for trying to build on top of somebody else who screwed up. That sucks.

>  - teach fsck to complain about parent commits being in the future from 
>    their children (allow the same small fudge factor).

And this is even worse.

But I think these are sane thing to do regardless. To prevent problematic commits from contaminating other people, we could add default post-receive hook and perhaps a new pre-merge hook to inspect the newly obtained history to warn and reject a merge (including fast-forward) if it contains commits far into the future. We would also need a mechanism to force a merge with such a clock-skewed history if we did so.

Show 6 quoted lines

>  - make the revision walking code realize that if times are too close to 
>    each other, it should walk a bit further back...
>
> because quite frankly, this bug only shows up when your time goes 
> backwards (or stays the same, but the fudge-factor should take care of 
> that too).

Show 16 quoted lines

> @@ -579,6 +581,15 @@ static int limit_list(struct rev_info *revs)
>  			return -1;
>  		if (obj->flags & UNINTERESTING) {
>  			mark_parents_uninteresting(commit);
> +
> +			/*
> +			 * If we have commits on the newlist, we don't
> +			 * want to do the "everybody_uninteresting()"
> +			 * test until we've hit a negative commit that
> +			 * is solidly in the past
> +			 */
> +			if (newlist && newlist->item->date < commit->date + FUDGE)
> +				continue;
>  			if (everybody_uninteresting(list))
>  				break;
>  			continue;

We picked up commit which we know is the youngest in the "list". Because we push into newlist as we traverse, newlist is sorted by date, and the element pointed by it is the youngest in there. That is something we have processed earlier, so it ought to be younger than this commit. Otherwise we have found a problematic clock skew. Ok, I think it should work.

The clock skew t6009 artificially introduces needs to be shortened for this, though.