Le 4 juin 2010 08:00, Junio C Hamano <gitster@xxxxxxxxx> a écrit :
> Axel Bonnet <axel.bonnet@xxxxxxxxxxxxxxx> 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().
> 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.
> 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);
>> 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.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html