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

From: Diane Gasselin <diane.gasselin@ensimag.imag.fr>
Date: Jun 4, 2010, 10:34 UTC
Message-ID: <AANLkTinfYAFAX4Hcl9lZMspju_Yk5eGll2S-z9HPdkjh@mail.gmail.com>
In-Reply-To: <7veign5oc9.fsf@alter.siamese.dyndns.org>

Le 4 juin 2010 08:00, Junio C Hamano <gitster@pobox.com> a écrit :

Show 25 quoted lines

> Axel Bonnet <axel.bonnet@ensimag.imag.fr> writes:
>
>> @@ -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;
>

We changed the structure, we now have something like:

case S_IFREG:
	if (DIFF_OPT_TST(&opt, ALLOW_TEXTCONV) &&
	    textconv_object(read_from, null_sha1, mode, &buf))
		;
	else if (strbuf_read_file(&buf, read_from, st.st_size) != st.st_size)
		die_errno("cannot open or read '%s'", read_from);
break;

> By the way, can't textconv_object() ever fail?  I see the function has its
> own die()

I don't really see which die you are talking about, there is no direct die in textconv_object().

Show 5 quoted lines

> 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.
>

The function fill_textconv() called by textconv_object() can fail if there is a problem during the textconv conversion. But textconv_object() tests before if we have to and if we can peform a conversion. We did not add some die but only let the existing die.

Show 13 quoted lines

> An alternative would be to encapsulate the whole of the above logic in one
> helper function perhaps.
>
>> @@ -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.
>

Actually, we call git_diff_ui_config in order to get the drivers. But
we could call a more specific configuration.
Here are others solutions:
1) We could add:

switch (userdiff_config(var, value)) {
	case 0: break;
	case -1: return -1;
	default: return 0;
}
to git_blame_config

2) We could call
git_config(git_diff_basic_config, NULL);
or git_config((config_fn_t)userdiff_config, NULL);
instead of git_config(git_diff_ui_config, NULL);

Show 11 quoted lines

>>       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.
>

Thanks a lot for your time and comments.