[RFCv2 PATCH 5/7] ui-blame: pull blame info from libgit

[whydoubt at gmail.com (Jeffrey Smith)]

On Sun, Sep 24, 2017 at 2:06 PM, Jeffrey Smith <whydoubt at gmail.com> wrote:
> A few of the original suggestions apparently got lost in the shuffle.
> Anyway, it appears that making your suggested structure change
> should clean some things up a lot.
>
> On Sat, Sep 23, 2017 at 10:47 AM, John Keeping <john at keeping.me.uk> wrote:
>> On Fri, Sep 22, 2017 at 10:38:46PM -0500, Jeff Smith wrote:
>>> Use the blame interface added in libgit to output the blame information
>>> of a file in the repository.
>>>
>>> Signed-off-by: Jeff Smith <whydoubt at gmail.com>
>>> ---
>>> diff --git a/ui-blame.c b/ui-blame.c
>>> index 901ca89..cc4457a 100644
>>> --- a/ui-blame.c
>>> +++ b/ui-blame.c
>>> @@ -10,6 +10,183 @@
>>>  #include "ui-blame.h"
>>>  #include "html.h"
>>>  #include "ui-shared.h"
>>> +#include "argv-array.h"
>>> +#include "blame.h"
>>> +
>>> +
>>> +/*
>>> + * Information on commits, used for output.
>>> + */
>>> +struct commit_info {
>>> +     struct strbuf author;
>>> +     struct strbuf author_mail;
>>> +     struct ident_split author_ident;
>>> +
>>> +     /* filled only when asked for details */
>>> +     struct strbuf committer;
>>> +     struct strbuf committer_mail;
>>> +     struct ident_split committer_ident;
>>> +
>>> +     struct strbuf summary;
>>> +};
>>
>> I think I commented on this last time, but I still don't understand why
>> this adds a new commit_info structure rather than reusing our existing
>> commitinfo and cgit_parse_commit(), especially since...
>>
>>> +static void get_commit_info(struct commit *commit,
>>> +                         struct commit_info *ret,
>>> +                         int detailed)
>>> +{
>>> +     int len;
>>> +     const char *subject, *encoding;
>>> +     const char *message;
>>> +
>>> +     commit_info_init(ret);
>>> +
>>> +     encoding = get_log_output_encoding();
>>
>> ... this is wrong as our output encoding has nothing to do with the
>> environment but is specified by PAGE_ENCODING.
>>
>>> +     message = logmsg_reencode(commit, NULL, encoding);
>>> +     get_ac_line(message, "\nauthor ",
>>> +                 &ret->author, &ret->author_mail,
>>> +                 &ret->author_ident);
>>> +
>>> +     if (!detailed) {
>>> +             unuse_commit_buffer(commit, message);
>>> +             return;
>>> +     }
>>> +
>>> +     get_ac_line(message, "\ncommitter ",
>>> +                 &ret->committer, &ret->committer_mail,
>>> +                 &ret->committer_ident);
>>> +
>>> +     len = find_commit_subject(message, &subject);
>>> +     if (len)
>>> +             strbuf_add(&ret->summary, subject, len);
>>> +     else
>>> +             strbuf_addf(&ret->summary, "(%s)", oid_to_hex(&commit->object.oid));
>>> +
>>> +     unuse_commit_buffer(commit, message);
>>> +}
>>> +
>>> +static char *emit_one_suspect_detail(struct blame_origin *suspect, const char *hex)
>>> +{
>>> +     struct commit_info ci;
>>> +     struct strbuf detail = STRBUF_INIT;
>>> +
>>> +     get_commit_info(suspect->commit, &ci, 1);
>>> +
>>> +     strbuf_addf(&detail, "author  %s", ci.author.buf);
>>> +     if (!ctx.cfg.noplainemail)
>>> +             strbuf_addf(&detail, " %s", ci.author_mail.buf);
>>> +     strbuf_addf(&detail, "  %s\n",
>>> +                 show_ident_date(&ci.author_ident,
>>> +                                 cgit_date_mode(DATE_ISO8601)));
>>> +
>>> +     strbuf_addf(&detail, "committer  %s", ci.committer.buf);
>>> +     if (!ctx.cfg.noplainemail)
>>> +             strbuf_addf(&detail, " %s", ci.committer_mail.buf);
>>> +     strbuf_addf(&detail, "  %s\n\n",
>>> +                 show_ident_date(&ci.committer_ident,
>>> +                                 cgit_date_mode(DATE_ISO8601)));
>>> +
>>> +     strbuf_addbuf(&detail, &ci.summary);
>>> +
>>> +     commit_info_destroy(&ci);
>>> +     return strbuf_detach(&detail, NULL);
>>> +}
>>> +
>>> +static void emit_blame_entry(struct blame_scoreboard *sb, struct blame_entry *ent)
>>> +{
>>> +     struct blame_origin *suspect = ent->suspect;
>>> +     char hex[GIT_SHA1_HEXSZ + 1];
>>> +     char *detail, *abbrev;
>>> +     unsigned long lineno;
>>> +     const char *numberfmt = "<a id='n%1$d' href='#n%1$d'>%1$d</a>\n";
>>> +     const char *cp, *cpend;
>>> +
>>> +     oid_to_hex_r(hex, &suspect->commit->object.oid);
>>> +     detail = emit_one_suspect_detail(suspect, hex);
>>> +     abbrev = xstrdup(find_unique_abbrev(suspect->commit->object.oid.hash,
>>> +                                         DEFAULT_ABBREV));
>>
>> nit: I don't think there's any need to strdup for abbrev, we use the
>> result immediately so the static buffer won't get overwritten.

But find_unique_abbrev() returns a const char*, and abbrev is passed
as the first parameter
to cgit_commit_link(), which is char* (and the function can in fact
change the contents).

I will add a patch to the series that avoids altering the first
parameter of cgit_commit_link
so that it will be const char*.

>>
>>> +     html("<tr><td class='lines'><pre>");
>>> +     cgit_commit_link(abbrev, detail, NULL, ctx.qry.head, hex, suspect->path);
>>> +     html("</pre></td>\n");
>>> +
>>> +     free(detail);
>>> +     free(abbrev);
>>> +
>>> +     if (ctx.cfg.enable_tree_linenumbers) {
>>> +             html("<td class='linenumbers'><pre>");
>>> +             lineno = ent->lno;
>>> +             while (lineno < ent->lno + ent->num_lines)
>>> +                     htmlf(numberfmt, ++lineno);
>>> +             html("</pre></td>\n");
>>> +     }
>>> +
>>> +     cp = blame_nth_line(sb, ent->lno);
>>> +     cpend = blame_nth_line(sb, ent->lno + ent->num_lines);
>>> +
>>> +     html("<td class='lines'><pre><code>");
>>> +     html_ntxt(cpend - cp, cp);
>>> +     html("</code></pre></td></tr>\n");
>>> +}
>>>
>>>  struct walk_tree_context {
>>>       char *curr_rev;
>>> @@ -47,10 +224,16 @@ static void set_title_from_path(const char *path)
>>>       strcat(new_title, ctx.page.title);
>>>       ctx.page.title = new_title;
>>>  }
>>> +
>>>  static void print_object(const unsigned char *sha1, const char *path, const char *basename, const char *rev)
>>>  {
>>>       enum object_type type;
>>>       unsigned long size;
>>> +     struct argv_array rev_argv = ARGV_ARRAY_INIT;
>>> +     struct rev_info revs;
>>> +     struct blame_scoreboard sb;
>>> +     struct blame_origin *o;
>>> +     struct blame_entry *ent = NULL;
>>>
>>>       type = sha1_object_info(sha1, &size);
>>>       if (type == OBJ_BAD) {
>>> @@ -59,7 +242,22 @@ static void print_object(const unsigned char *sha1, const char *path, const char
>>>               return;
>>>       }
>>>
>>> -     /* Read in applicable data here */
>>> +     argv_array_push(&rev_argv, "blame");
>>> +     argv_array_push(&rev_argv, rev);
>>> +     init_revisions(&revs, NULL);
>>> +     DIFF_OPT_SET(&revs.diffopt, ALLOW_TEXTCONV);
>>> +     setup_revisions(rev_argv.argc, rev_argv.argv, &revs, NULL);
>>> +     init_scoreboard(&sb);
>>> +     sb.revs = &revs;
>>> +     setup_scoreboard(&sb, path, &o);
>>> +     o->suspects = blame_entry_prepend(NULL, 0, sb.num_lines, o);
>>> +     prio_queue_put(&sb.commits, o->commit);
>>> +     blame_origin_decref(o);
>>> +     sb.ent = NULL;
>>> +     sb.path = path;
>>> +     assign_blame(&sb, 0);
>>> +     blame_sort_final(&sb);
>>> +     blame_coalesce(&sb);
>>>
>>>       set_title_from_path(path);
>>>
>>> @@ -76,7 +274,15 @@ static void print_object(const unsigned char *sha1, const char *path, const char
>>>               return;
>>>       }
>>>
>>> -     /* Output data here */
>>> +     html("<table class='blame blob'>");
>>> +     for (ent = sb.ent; ent; ) {
>>> +             struct blame_entry *e = ent->next;
>>> +             emit_blame_entry(&sb, ent);
>>> +             free(ent);
>>> +             ent = e;
>>> +     }
>>> +     html("</table>\n");
>>> +     free((void *)sb.final_buf);
>>>
>>>       cgit_print_layout_end();
>>>       return;