Re: [PATCH v4 08/18] map/take range to the parent of commits
- From
Thomas Rast <trast@student.ethz.ch>
- Date
- Aug 5, 2010, 21:32 UTC
- Message-ID
- <201008052332.37435.trast@student.ethz.ch>
- In-Reply-To
- <1281024717-7855-9-git-send-email-struggleyb.nku@gmail.com>
Bo Yang wrote:
> The algorithm that maps lines from post-image to pre-image is in > the function map_lines. Generally, we use simple line number > calculation method to do the map.
> +#define SCALE_FACTOR 4 > +/* > + * [p_start, p_end] represents the pre-image of current diff hunk, > + * [t_start, t_end] represnets the post-image of the current diff hunk,
^^ Typo here ------------------/
Show 8 quoted lines
> + * [start, end] represents the currently interesting line range in
> + * post-image,
> + * [o_start, o_end] represents the original line range that coresponds
> + * to current line range.
> + */
> +void map_lines(long p_start, long p_end, long t_start, long t_end,
> + long start, long end, long *o_start, long *o_end)
> +{[...]
Show 10 quoted lines
> + /*
> + * A heuristic for lines mapping:
> + *
> + * When the pre-image is no more than 1/4 of the post-image,
> + * there is no effective way to find out which part of pre-image
> + * corresponds to the currently interesting range of post-image.
> + * And we are in the danger of tracking totally useless lines.
> + * So, we just treat all the post-image lines as added from scratch.
> + */
> + if (SCALE_FACTOR * (p_end - p_start + 1) < (t_end - t_start + 1)) {So that's what SCALE_FACTOR is good for (and the comment should probably say 1/SCALE_FACTOR instead).
Out of curiosity, did you come up with 4 randomly or by experimentation?
Show 5 quoted lines
> +/* > + * When same == 1: > + * [p_start, p_end] represents the diff hunk line range of pre-image, > + * [t_start, t_end] represents the diff hunk line range of post-image. > + * When same == 0, they represents a range of idnetical lines between
+ * When same == 0, they represent a range of identical lines between
Show 7 quoted lines
> + * two images. > + * > + * This function find out the corresponding line ranges of currently > + * interesting ranges which this diff hunk touches. > + */ > +static void map_range(struct take_range_cb_data *data, int same, > + long p_start, long p_end, long t_start, long t_end)
You took some time to comment map_lines, but not this one, sadly.
I gather it works as
assign_parents_range -> assign_range_to_parent once with map=1, once with map=0 -> either map_range_cb or take_range_cb -> either map_range or take_range
but there are few comments on where the decisions should be obvious and where they are just heuristics. Can you add some more comments to enlighten us?
> + if (map) > + map_range(&cb, 1, cb.plno + 1, 0x7FFFFFFF, cb.tlno + 1, 0x7FFFFFFF); > + else > + take_range(&cb, cb.plno + 1, 0x7FFFFFFF, cb.tlno + 1, 0x7FFFFFFF);
Use INT_MAX from limits.h (and besides, you're not guaranteed to have 32 bits).
Show 9 quoted lines
> + /*
> + * Loop on the parents and assign the ranges to different
> + * parents, if there is any range left, this commit must
> + * be an evil merge.
> + */
> + copy = diff_line_range_clone_deeply(r);
> + parents = commit->parents;
> + while (parents) {
> + struct commit *p = parents->item;> + assign_range_to_parent(rev, commit, p, r, &rev->diffopt, 1);
IIUC, the latter line is
/* assign to the parent what we can */
and the next one
> + assign_range_to_parent(rev, commit, p, copy, &rev->diffopt, 0);
/* and remove it from our to-be-printed range */
right?
If so, please rename the 'copy' variable since its purpose is not to be a copy, but to hold entirely different data.
--
Thomas Rast
trast@{inf,student}.ethz.ch