Re: [RFC/PATCH 3/4] textconv: support for blame
- From
Jeff King <peff@peff.net>
- Date
- Jun 6, 2010, 21:51 UTC
- Message-ID
- <20100606215159.GA6993@coredump.intra.peff.net>
- In-Reply-To
- <1275562038-7468-4-git-send-email-axel.bonnet@ensimag.imag.fr>
On Thu, Jun 03, 2010 at 12:47:17PM +0200, Axel Bonnet wrote:
Show 20 quoted lines
> /*
> + * Prepare diff_filespec and convert it using diff textconv API
> + * if the textconv driver exists.
> + * Return 1 if the conversion succeeds, 0 otherwise.
> + */
> +static int textconv_object(const char *path,
> + const unsigned char *sha1,
> + unsigned short mode,
> + struct strbuf *buf)
> +{
> + struct diff_filespec *df;
> +
> + df = alloc_filespec(path);
> + fill_filespec(df, sha1, mode);
> + get_textconv(df);
> +
> + if (!df->driver|| !df->driver->textconv) {
> + free_filespec(df);
> + return 0;
> + }df->driver is always non-NULL these days, isn't it? Also, why not just use the return value of get_textconv, which does handles this conditional for you already (and avoids peeking directly at df->driver, which was what get_textconv was meant to abstract). I.e.:
struct userdiff_driver *textconv;
...
textconv = get_textconv(df);
if (!textconv)
... free and return ...> + buf->len = fill_textconv(df->driver, df, &buf->buf); > + buf->alloc = 1; > + free_filespec(df); > + return 1;
Shoving the allocated buffer into a strbuf really feels like an abuse of strbuf. I don't think there is any bug here (the original buffer actually comes from a strbuf, and by setting alloc to 1, you indicate that any further appending would need to realloc), but it just seems like violating the boundary of the strbuf API.
And it isn't really necessary because:
> + if (DIFF_OPT_TST(&opt, ALLOW_TEXTCONV) && > + textconv_object(o->path, o->blob_sha1, mode, &buf)) > + file->ptr = strbuf_detach(&buf, (size_t *) &file->size);
You just end up pulling it out into an mmfile_t, anyway. So why involve strbuf at all?
Show 22 quoted lines
> static void fill_origin_blob(struct diff_options opt,
> struct origin *o, mmfile_t *file)
> {
> + unsigned mode;
> +
> if (!o->file.ptr) {
> + struct strbuf buf = STRBUF_INIT;
> enum object_type type;
> num_read_blob++;
> - file->ptr = read_sha1_file(o->blob_sha1, &type,
> - (unsigned long *)(&(file->size)));
> +
> + get_tree_entry(o->commit->object.sha1,
> + o->path,
> + o->blob_sha1, &mode);
> + if (DIFF_OPT_TST(&opt, ALLOW_TEXTCONV) &&
> + textconv_object(o->path, o->blob_sha1, mode, &buf))
> + file->ptr = strbuf_detach(&buf, (size_t *) &file->size);
> + else
> + file->ptr = read_sha1_file(o->blob_sha1, &type,
> + (unsigned long *)(&(file->size)));
> + strbuf_release(&buf);I don't understand why there's a get_tree_entry call here. Don't we already have the path and blob_sha1 fields? Is the mode actually relevant (i.e., can we just fake it for the purposes of the diff_filespec we will create)?
Even if we do need the get_tree_entry call, shouldn't it happen only if we are allowing textconv, so non-textconv users don't pay for the call?
Show 5 quoted lines
> @@ -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);
Like Junio, I am worried about the unintended consequences of git_diff_ui_config. The userdiff drivers are parsed as part of git_diff_basic_config, and you should use that.
Also, you are better to just add the call to git_blame_config (either git_diff_basic_config, or looking directly to userdiff_config) instead of calling git_config again, which will avoid parsing the config files twice.
-Peff