Re: [PATCH] Add git-annotate, a tool for assigning blame.

From: Ryan Anderson <ryan@michonline.com>
Date: Feb 23, 2006, 22:10 UTC
Message-ID: <20060223221048.GA6423@mythryan2.michonline.com>
In-Reply-To: <20060220234054.GA7903@c165.ib.student.liu.se>

(Biased critique since I have the other tool in the tree, but still...)

On Tue, Feb 21, 2006 at 12:40:54AM +0100, Fredrik Kuivinen wrote:

Show 65 quoted lines

> diff --git a/blame.c b/blame.c
> new file mode 100644
> index 0000000..d4a2fad
> --- /dev/null
> +++ b/blame.c
> @@ -0,0 +1,444 @@
> +#include <assert.h>
> +
> +#include "cache.h"
> +#include "refs.h"
> +#include "tag.h"
> +#include "commit.h"
> +#include "tree.h"
> +#include "blob.h"
> +#include "epoch.h"
> +#include "diff.h"
> +
> +#define DEBUG 0
> +
> +struct commit** blame_lines;
> +int num_blame_lines;
> +
> +struct util_info
> +{
> +    int* line_map;
> +    int num_lines;
> +    unsigned char sha1[20]; /* blob sha, not commit! */
> +    char* buf;
> +    unsigned long size;
> +//    const char* path;
> +};
> +
> +struct chunk
> +{
> +    int off1, len1; // ---
> +    int off2, len2; // +++
> +};
> +
> +struct patch
> +{
> +    struct chunk* chunks;
> +    int num;
> +};
> +
> +static void get_blob(struct commit* commit);
> +
> +int num_get_patch = 0;
> +int num_commits = 0;
> +
> +struct patch* get_patch(struct commit* commit, struct commit* other)
> +{
> +    struct patch* ret = xmalloc(sizeof(struct patch));
> +    ret->chunks = NULL;
> +    ret->num = 0;
> +
> +    struct util_info* info_c = (struct util_info*) commit->object.util;
> +    struct util_info* info_o = (struct util_info*) other->object.util;
> +
> +    if(!memcmp(info_c->sha1, info_o->sha1, 20))
> +        return ret;
> +
> +    get_blob(commit);
> +    get_blob(other);
> +
> +    FILE* fout = fopen("/tmp/git-blame-tmp1", "w");

Probably should be using something like mkstemp (mkstmp?) here, so if someone is runnign things as root or with a malicous user around, things don't collide.

Hell, so on a multi-user machine this doesn't blow up on you.

But, read down for a related comment.

Show 18 quoted lines

> +    if(!fout)
> +        die("fopen tmp1 failed: %s", strerror(errno));
> +
> +    if(fwrite(info_c->buf, info_c->size, 1, fout) != 1)
> +        die("fwrite 1 failed: %s", strerror(errno));
> +    fclose(fout);
> +
> +    fout = fopen("/tmp/git-blame-tmp2", "w");
> +    if(!fout)
> +        die("fopen tmp2 failed: %s", strerror(errno));
> +
> +    if(fwrite(info_o->buf, info_o->size, 1, fout) != 1)
> +        die("fwrite 2 failed: %s", strerror(errno));
> +    fclose(fout);
> +
> +    FILE* fin = popen("diff -u0 /tmp/git-blame-tmp1 /tmp/git-blame-tmp2", "r");
> +    if(!fin)
> +        die("popen failed: %s", strerror(errno));

Can't git-diff-tree do this sufficiently, anyway? See my Perl script for an example, you just need both commit IDs and both filenames and the appropriate -M and you get the right results.

(It's possible that's part of where the performance differences are, though, not really sure at the moment.)

I'm going to stop there for the moment, I'm not really confident in my understanding of git-internals to say much more just yet.

This could probably benefit a *LOT* from the libification project, I think, though.

-- 
Ryan Anderson
  sometimes Pug Majere