[PATCH 1/1] enable cgit to show gravatar for author, committer and tagger

[Peter Wu]

Hi,

See below for review.

On Wednesday 27 November 2013 16:17:17 Christian Hesse wrote:
> ---
>  cgit.c       |  3 +++
>  cgit.h       |  5 +++++
>  cgitrc.5.txt |  4 ++++
>  parsing.c    | 47 +++++++++++++++++++++++++++++++++++++++++++++++
>  shared.c     |  6 ++++++
>  ui-commit.c  | 10 ++++++++++
>  ui-log.c     |  5 +++++
>  ui-refs.c    | 10 ++++++++++
>  ui-tag.c     |  5 +++++
>  9 files changed, 95 insertions(+)

[..]

> @@ -258,3 +273,35 @@ struct taginfo *cgit_parse_tag(struct tag *tag)
>  	free(data);
>  	return ret;
>  }
> +
> +char * cgit_get_gravatar(const char * email) {
> +    	int n, length;
> +	MD5_CTX c;
> +	unsigned char digest[16];
> +	char hex[33];
> +	char * gravatar = malloc(67);
> +
> +	/* skip brackets! */
> +	email++;
> +	length = strlen(email) - 1;

> +
> +	MD5_Init(&c);
> +
> +	while (length > 0) {
> +		if (length > 512)
> +			MD5_Update(&c, email, 512);
> +		else
> +			MD5_Update(&c, email, length);
Seems like an error from Stackoverflow[1]? According to the manual page, the 
length is in bytes, not bits.

 [1]: http://stackoverflow.com/a/8389763/427545

> +		length -= 512;
> +		email += 512;
> +	}
> +
> +	MD5_Final(digest, &c);

Since you know the full string onbeforehand, you should also be able to use 
MD5() directly. Something like (openssl):

    MD5(email, length, digest);

As you mentioned, email addresses should be converted to lower case first 
according to http://en.gravatar.com/site/implement/hash/. The below str_to_hex 
was written before I knew that.

> +
> +	for (n = 0; n < 16; ++n)
> +		snprintf(&(hex[n*2]), 16*2, "%02x", (unsigned int)digest[n]);

This 16*2 is bogus, if you are at the 30th position, you only have two bytes 
left. What about adding a new md5_to_hex() function, similar to sha1_to_hex() 
in git? Here is a generic approach:

/* gets a string representing the binary data. dst_str must have a
 * size of at least src_len * 2 + 1 bytes. */
    void str_to_hex(char *dst_str, const unsigned char *src_bytes,
                    size_t src_len) {
        const char *hex = "0123456789abcdef";
        int n;

        for (n = 0; n < src_len; ++n) {
            unsigned char val = *src_bytes++;
            *dst_str++ = hex[val >> 4];
            *dst_str++ = hex[val & 0xf];
        }

        *dst_str = 0;
    }

    str_to_hex(hex, digest, 16);

The emailaddress limitation could probably be resolved here by repeatedly 
calling:

    MD5_Update(&c, tolower(email[n]), 1)

> +
> +	sprintf(gravatar, "http://www.gravatar.com/avatar/%s?s=", hex);

Use "//www.gravatar.com/.." instead for HTTPS sites?

> +
> +	return gravatar;
> +}

[..]

> --- a/ui-commit.c
> +++ b/ui-commit.c
> @@ -44,6 +44,11 @@ void cgit_print_commit(char *hex, const char *prefix)
>  	cgit_print_diff_ctrls();
>  	html("<table summary='commit info' class='commit-info'>\n");
>  	html("<tr><th>author</th><td>");
> +	if (ctx.cfg.enable_gravatar && info->author_gravatar) {
> +		html("<img src='");
> +		html(info->author_gravatar);
> +		html("' width=16 height=16 alt='Gravatar' \\>");
> +	}
>  	html_txt(info->author);
>  	if (!ctx.cfg.noplainemail) {
>  		html(" ");
> @@ -53,6 +58,11 @@ void cgit_print_commit(char *hex, const char *prefix)
>  	cgit_print_date(info->author_date, FMT_LONGDATE, ctx.cfg.local_time);
>  	html("</td></tr>\n");
>  	html("<tr><th>committer</th><td>");
> +	if (ctx.cfg.enable_gravatar && info->committer_gravatar) {
> +		html("<img src='");
> +		html(info->committer_gravatar);
> +		html("' width=16 height=16 alt='Gravatar' \\>");

SHouldn't that \\ become a / for XHTML? 

> +	}
>  	html_txt(info->committer);
>  	if (!ctx.cfg.noplainemail) {
>  		html(" ");
> diff --git a/ui-log.c b/ui-log.c
> index 6f1249b..9974139 100644
> --- a/ui-log.c
> +++ b/ui-log.c
> @@ -168,6 +168,11 @@ static void print_commit(struct commit *commit, struct
> rev_info *revs) sha1_to_hex(commit->object.sha1), ctx.qry.vpath, 0);
>  	show_commit_decorations(commit);
>  	html("</td><td>");
> +	if (ctx.cfg.enable_gravatar && info->author_gravatar) {
> +		html("<img src='");
> +		html(info->author_gravatar);
> +		html("' width=16 height=16 alt='Gravatar' \\>");

Same here, "/>" instead of "\>".

> +	}
>  	html_txt(info->author);
> 
>  	if (revs->graph) {
> diff --git a/ui-refs.c b/ui-refs.c
> index 0ae0612..8714b2d 100644
> --- a/ui-refs.c
> +++ b/ui-refs.c
> @@ -77,6 +77,11 @@ static int print_branch(struct refinfo *ref)
>  	if (ref->object->type == OBJ_COMMIT) {
>  		cgit_commit_link(info->subject, NULL, NULL, name, NULL, NULL, 0);
>  		html("</td><td>");
> +		if (ctx.cfg.enable_gravatar && info->author_gravatar) {
> +			html("<img src='");
> +			html(info->author_gravatar);
> +			html("' width=16 height=16 alt='Gravatar' \\>");

Same..

> +		}
>  		html_txt(info->author);
>  		html("</td><td colspan='2'>");
>  		cgit_print_age(info->commit->date, -1, NULL);
> @@ -154,6 +159,11 @@ static int print_tag(struct refinfo *ref)
>  		cgit_object_link(obj);
>  	html("</td><td>");
>  	if (info) {
> +		if (ctx.cfg.enable_gravatar && info->tagger_gravatar) {
> +			html("<img src='");
> +			html(info->tagger_gravatar);
> +			html("' width=16 height=16 alt='Gravatar' \\>");

Same..

> +		}
>  		if (info->tagger)
>  			html(info->tagger);
>  	} else if (ref->object->type == OBJ_COMMIT) {
> diff --git a/ui-tag.c b/ui-tag.c
> index aea7958..ad402da 100644
> --- a/ui-tag.c
> +++ b/ui-tag.c
> @@ -77,6 +77,11 @@ void cgit_print_tag(char *revname)
>  		}
>  		if (info->tagger) {
>  			html("<tr><td>tagged by</td><td>");
> +			if (ctx.cfg.enable_gravatar && info->tagger_gravatar) {
> +				html("<img src='");
> +				html(info->tagger_gravatar);
> +				html("' width=16 height=16 alt='Gravatar' \\>");

Yep, also wrong slashes.

> +			}
>  			html_txt(info->tagger);
>  			if (info->tagger_email && !ctx.cfg.noplainemail) {
>  				html(" ");

Regards,
Peter