Re: [RFC/PATCH 2/4] limit "contains" traversals based on commit timestamp

From: Jonathan Nieder <jrnieder@gmail.com>
Date: Oct 13, 2010, 23:21 UTC
Message-ID: <20101013232153.GC11793@burratino>
In-Reply-To: <20100705123419.GB25699@sigill.intra.peff.net>

Jeff King wrote:

Show 7 quoted lines

> Name-rev already implements a similar optimization, using a
> "slop" of one day to allow for a certain amount of clock
> skew in commit timestamps. This patch introduces a
> "core.clockskew" variable, which allows specifying the
> allowable amount of clock skew in seconds.  For safety, it
> defaults to "none", causing a full traversal (i.e., no
> change in behavior from previous versions).

Tests?

Actually just a short example script to try would be helpful, if anyone has one handy (yes, I am terribly lazy). Such a script would be useful for figuring out which commands ought to be updated to respect core_clock_skew. rev-list is one.

> --- a/commit.c
> +++ b/commit.c

[...]

Show 8 quoted lines

> @@ -872,9 +874,13 @@ static int contains_recurse(struct commit *candidate,
>  	if (parse_commit(candidate) < 0)
>  		return 0;
>  
> +	/* stop searching if we go too far back in time */
> +	if (candidate->date < cutoff)
> +		return 0;
> +

Nice idea.

Show 13 quoted lines

> @@ -885,5 +891,20 @@ static int contains_recurse(struct commit *candidate,
>  
>  int contains(struct commit *candidate, const struct commit_list *want)
>  {
> -	return contains_recurse(candidate, want);
> +	unsigned long cutoff = 0;
> +
> +	if (core_clock_skew >= 0) {
> +		const struct commit_list *c;
> +		unsigned long min_date = ULONG_MAX;
> +		for (c = want; c; c = c->next) {
> +			if (parse_commit(c->item) < 0)
> +				continue;

Why ignore these errors?  Will they be noticed later?

The rest of the patch looks good to me. I am not thrilled with making the user figure out an acceptable "[core] clockskew" value (and am not sure it makes much sense as a tunable setting), but it is better than the status quo, so...