Re: [PATCH] Require 0 context lines in git-blame algorithm
- From
Junio C Hamano <gitster@pobox.com>
- Date
- May 27, 2016, 20:59 UTC
- Message-ID
- <xmqqtwhjp694.fsf@gitster.mtv.corp.google.com>
- In-Reply-To
- <1464358592-5409-1-git-send-email-dak@gnu.org>
David Kastrup <dak@gnu.org> writes:
Show 5 quoted lines
> Previously, the core part of git blame -M required 1 context line. > There is no rationale to be found in the code (one guess would be that > the old blame algorithm was unable to deal with immediately adjacent > regions), and it causes artifacts like discussed in the thread > <URL:http://thread.gmane.org/gmane.comp.version-control.git/255289/>
The only thing that remotely hints why we thought a non-zero context was a good idea was this:
http://thread.gmane.org/gmane.comp.version-control.git/28336/focus=28580
in which I said:
| we may need to use a handful surrounding context lines for | better identification of copy source by the "ciff" algorithm but | that is a minor implementation detail.
But I do not think the amount of context affects the quality of the match. So it could be that it was completely a misguided attempt since the very beginning, cee7f245 (git-pickaxe: blame rewritten., 2006-10-19), which allowed the caller to specify context when calling compare_buffer(), the function that corresponds to diff_hunks() in today's code.
> --- > builtin/blame.c | 7 +++---- > 1 file changed, 3 insertions(+), 4 deletions(-)
I totally forgot about the discussion around $gmane/255289; thanks for bringing this back again.
As usual, we'd need your sign-off to use this patch.
Thanks.
Show 39 quoted lines
> diff --git a/builtin/blame.c b/builtin/blame.c
> index 21f42b0..a3f6874 100644
> --- a/builtin/blame.c
> +++ b/builtin/blame.c
> @@ -134,7 +134,7 @@ struct progress_info {
> int blamed_lines;
> };
>
> -static int diff_hunks(mmfile_t *file_a, mmfile_t *file_b, long ctxlen,
> +static int diff_hunks(mmfile_t *file_a, mmfile_t *file_b,
> xdl_emit_hunk_consume_func_t hunk_func, void *cb_data)
> {
> xpparam_t xpp = {0};
> @@ -142,7 +142,6 @@ static int diff_hunks(mmfile_t *file_a, mmfile_t *file_b, long ctxlen,
> xdemitcb_t ecb = {NULL};
>
> xpp.flags = xdl_opts;
> - xecfg.ctxlen = ctxlen;
> xecfg.hunk_func = hunk_func;
> ecb.priv = cb_data;
> return xdi_diff(file_a, file_b, &xpp, &xecfg, &ecb);
> @@ -980,7 +979,7 @@ static void pass_blame_to_parent(struct scoreboard *sb,
> fill_origin_blob(&sb->revs->diffopt, target, &file_o);
> num_get_patch++;
>
> - if (diff_hunks(&file_p, &file_o, 0, blame_chunk_cb, &d))
> + if (diff_hunks(&file_p, &file_o, blame_chunk_cb, &d))
> die("unable to generate diff (%s -> %s)",
> oid_to_hex(&parent->commit->object.oid),
> oid_to_hex(&target->commit->object.oid));
> @@ -1129,7 +1128,7 @@ static void find_copy_in_blob(struct scoreboard *sb,
> * file_p partially may match that image.
> */
> memset(split, 0, sizeof(struct blame_entry [3]));
> - if (diff_hunks(file_p, &file_o, 1, handle_split_cb, &d))
> + if (diff_hunks(file_p, &file_o, handle_split_cb, &d))
> die("unable to generate diff (%s)",
> oid_to_hex(&parent->commit->object.oid));
> /* remainder, if any, all match the preimage */