Re: [PATCH] Improve parent blame to detect renames by using the previous information

From: Jeff King <peff@peff.net>
Date: Jun 6, 2010, 22:35 UTC
Message-ID: <20100606223545.GA11424@coredump.intra.peff.net>
In-Reply-To: <1275767765-8509-1-git-send-email-fonseca@diku.dk>

On Sat, Jun 05, 2010 at 03:56:05PM -0400, Jonas Fonseca wrote:

>  I finally got some more time to dig around this. What if we simply uses
>  the information given by the porcelain output's previous line? It
>  handles your simple test case, and navigating in the tig repository. It
>  also makes it possible to delete a lot of code.

Yes, I think that is the right way to go. The whole time I was doing the other patches, I kept thinking that we had something like this in the blame output, but when I looked I couldn't find it (which I can't see how I would manage now, it's quite obvious to see).

So I think it does the right thing, and I see you also included my fix:

Show 17 quoted lines

> +	char from[SIZEOF_REF + SIZEOF_STR];
> +	char to[SIZEOF_REF + SIZEOF_STR];
>  	const char *diff_tree_argv[] = {
> -		"git", "diff-tree", "-U0", blame->commit->id,
> -			"--", blame->commit->filename, NULL
> +		"git", "diff", "--no-textconv", "--no-extdiff", "--no-color",
> +			"-U0", from, to, "--", NULL
>  	};
>  	struct io io;
>  	int parent_lineno = -1;
>  	int blamed_lineno = -1;
>  	char *line;
>  
> +	snprintf(from, sizeof(from), "%s:%s", opt_ref, opt_file);
> +	snprintf(to, sizeof(to), "%s:%s", blame->commit->id,
> +		 blame->commit->filename);
> +

to handle the line-jumping properly.

One minor bug:

Show 15 quoted lines

> @@ -5204,10 +5148,13 @@ blame_request(struct view *view, enum request request, struct line *line)
>  		break;
>  
>  	case REQ_PARENT:
> -		if (check_blame_commit(blame, TRUE) &&
> -		    select_commit_parent(blame->commit->id, opt_ref,
> -					 blame->commit->filename)) {
> -			string_copy(opt_file, blame->commit->filename);
> +		if (!check_blame_commit(blame, TRUE))
> +			break;
> +		if (!*blame->commit->parent_id) {
> +			report("The selected commit has no parents");
> +		} else {
> +			string_copy_rev(opt_ref, blame->commit->parent_id);
> +			string_copy_rev(opt_file, blame->commit->parent_filename);

This second string_copy_rev should be a string_ncopy, shouldn't it?

-Peff