Re: [PATCH v7 5/5] Implement line-history search (git log -L)

From: Thomas Rast <trast@inf.ethz.ch>
Date: Jun 7, 2012, 17:52 UTC
Message-ID: <87sje757sl.fsf@thomas.inf.ethz.ch>
In-Reply-To: <7vhaunhvc8.fsf@alter.siamese.dyndns.org>

Junio C Hamano <gitster@pobox.com> writes:

Show 7 quoted lines

> Thomas Rast <trast@student.ethz.ch> writes:
>
>> This is a rewrite of much of Bo's work, mainly in an effort to split
>> it into smaller, easier to understand routines.
>
> You mentioned "splitting" in the cover letter, but it does seem to
> need a bit more work.

Yes, I think I also mentioned that it's not ready for inclusion ;-)

Most of your points are spot on, however:

Show 7 quoted lines

>> +static void diff_ranges_release (struct diff_ranges *diff)
>> +{
>> +	range_set_release(&diff->parent);
>> +	range_set_release(&diff->target);
>> +}
>
> Unused.

That should end up being used a few times...

>> +static void diff_ranges_filter_touched (struct diff_ranges *out,
>> +					struct diff_ranges *diff,
>> +					struct range_set *rs)

...

Show 9 quoted lines

>> +		if (ranges_overlap(&diff->target.ranges[i], &rs->ranges[j])) {
>> +			range_set_append(&out->parent,
>> +					 diff->parent.ranges[i].start,
>> +					 diff->parent.ranges[i].end);
>> +			range_set_append(&out->target,
>> +					 diff->target.ranges[i].start,
>> +					 diff->target.ranges[i].end);
>
> Shouldn't the ranges be merged, not just appended?

If the code ever passed anything but an empty struct diff_ranges as the 'out' argument, yes. But it doesn't. In general I'm usually doing the 'out' dance to save one heap allocation. Perhaps it would be cleaner to allocate all of them on the heap instead, and return as pointers, dunno.

>> +	/* line level range that we are chasing */
>> +	struct decoration line_log_data;
>
> Good use of decoration.

That was actually Bo's idea.

-- 
Thomas Rast
trast@{inf,student}.ethz.ch