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

From: Fredrik Kuivinen <freku045@student.liu.se>
Date: Feb 23, 2006, 22:55 UTC
Message-ID: <20060223225547.GB8673@c165.ib.student.liu.se>
In-Reply-To: <20060223221048.GA6423@mythryan2.michonline.com>

On Thu, Feb 23, 2006 at 05:10:49PM -0500, Ryan Anderson wrote:

Show 9 quoted lines

> (Biased critique since I have the other tool in the tree, but still...)
> 
> > +    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.

Yep, I know. The code is mostly a proof of concept. I didn't submit it for inclusion.

Show 29 quoted lines

> 
> But, read down for a related comment.
> 
> > +    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.)
>

Yeah.. maybe. My first thought was to avoid forking and execing diff and use some C library for doing the diffing instead (libxdiff). But then I just wanted to get some code working and the simplest solution I could think of was to fork and exec diff.

Show 5 quoted lines

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

Yes, perhaps. Some of the git-rev-list bits might simplify a couple of things.

I have found some severe problems with the code I posted, in particular it doesn't handle parallel development tracks at all. I am working on fixing it, but it isn't finished yet.

Thanks for the comments.

- Fredrik