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