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

[Jonathan Nieder <jrnieder@gmail.com>]

Jeff King wrote:

> 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
[...]
> @@ -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.

> @@ -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...