From mboxrd@z Thu Jan  1 00:00:00 1970
From: whydoubt at gmail.com (Jeffrey Smith)
Date: Sun, 24 Sep 2017 14:06:02 -0500
Subject: [RFCv2 PATCH 5/7] ui-blame: pull blame info from libgit
In-Reply-To: <20170923154758.GE2548@john.keeping.me.uk>
References: <20170608021810.12964-1-whydoubt@gmail.com>
 <20170923033848.5922-1-whydoubt@gmail.com>
 <20170923033848.5922-6-whydoubt@gmail.com>
 <20170923154758.GE2548@john.keeping.me.uk>
Message-ID: <CAPX7N=4n2VFrX3DGZmkVdS2+4QLyE04t7XhXnDELKd7i5X3KxQ@mail.gmail.com>

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