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

Le 4 juin 2010 08:00, Junio C Hamano [off-list ref] a écrit :

Axel Bonnet [off-list ref] writes:

quoted

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

quoted

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

quoted

      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.