Re: [RFC/PATCH 3/4] textconv: support for blame
From: Junio C Hamano <hidden>
Date: 2016-06-15 22:48:54
Axel Bonnet [off-list ref] writes:
quoted hunk ↗ jump to hunk
@@ -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.
quoted hunk ↗ jump to hunk
@@ -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.
quoted hunk ↗ jump to hunk
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.