Re: [RFC/PATCH 3/4] textconv: support for blame

From: Junio C Hamano <gitster@pobox.com>
Date: Jun 4, 2010, 06:00 UTC
Message-ID: <7veign5oc9.fsf@alter.siamese.dyndns.org>
In-Reply-To: <1275562038-7468-4-git-send-email-axel.bonnet@ensimag.imag.fr>

Axel Bonnet <axel.bonnet@ensimag.imag.fr> writes:

Show 13 quoted lines

> @@ -2033,10 +2072,13 @@ static struct commit *fake_working_tree_commit(struct diff_options opt,
>  			read_from = path;
>  		}
>  		mode = canon_mode(st.st_mode);
> +
>  		switch (st.st_mode & S_IFMT) {
>  		case S_IFREG:
> -			if (strbuf_read_file(&buf, read_from, st.st_size) != st.st_size)
> -				die_errno("cannot open or read '%s'", read_from);
> +			if (!DIFF_OPT_TST(&opt, ALLOW_TEXTCONV) ||
> +			    !textconv_object(read_from, null_sha1, mode, &buf))
> +				if (strbuf_read_file(&buf, read_from, st.st_size) != st.st_size)
> +					die_errno("cannot open or read '%s'", read_from);

This is just a style thing but it would probably be easier to read if you structured it like:

	if (! we are allowed to use textconv ||
	    do textconv and we did get the converted data in the buffer)
		; /* happy */
	else if (! successfully read the blob into buffer)
		die;

By the way, can't textconv_object() ever fail? I see the function has its own die() but it looks a bit funny to see one branch of an "if" statement calls a function that lets the caller decide to die while the function called by the other branch unconditionally dies on failure at the API design level.

An alternative would be to encapsulate the whole of the above logic in one helper function perhaps.

Show 5 quoted lines

> @@ -2249,8 +2291,10 @@ int cmd_blame(int argc, const char **argv, const char *prefix)
>  	int cmd_is_annotate = !strcmp(argv[0], "annotate");
>  
>  	git_config(git_blame_config, NULL);
> +	git_config(git_diff_ui_config, NULL);

What configuration are we pulling into the system with this call? Would they ever affect the internal diff machinery in a negative way? I am especially wondering about "diff.renames" here.

>  	init_revisions(&revs, NULL);
>  	revs.date_mode = blame_date_mode;
> +	DIFF_OPT_SET(&revs.diffopt, ALLOW_TEXTCONV);

As an RFC patch, I would have preferred if we didn't have this line to force --textconv on by default, but instead you merely allowed the mechanism to be used by giving the option explicitly from the command line.

Other than these points, the series looked quite sane to me.