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

From: Junio C Hamano <junkio@cox.net>
Date: Feb 20, 2006, 22:54 UTC
Message-ID: <7virr98wvc.fsf@assigned-by-dhcp.cox.net>
In-Reply-To: <11404323692193-git-send-email-ryan@michonline.com>

Ryan Anderson <ryan@michonline.com> writes:

> I would appreciate some other testing on this, as I can't find a case
> where it falls down, but the files with a lot of history tend to have a
> lot of lines, making them hard to spotcheck without having been an
> intimate part of that history.

I looked at a couple of files, including pack-objects.c, rev-list.c (in "pu"), and svnimport.perl; what I saw made sense. I think however we would want to check things with a lot of merges, so git repository may not be a good guinea pig.

> Oh, this is the "functional" version, but it might not qualify as "nice
> looking" yet, pleaes, feel free to complain.

Nice.

Two design glitches and an implementation:

 - You seem to rely on the working tree file to be clean relative to
   HEAD.

 - You do not take anything other than HEAD as the starting point.

Maybe an additional -r<this-version> option which defaults to HEAD, plus reading the blob always from that tree as the starting point would be helpful.

> +	open(P,"-|","git-rev-list","--parents","--remove-empty",$rev,"--",$file)

Johannes noticed I slipped this form which Perl 5.6 does not grok in another program. Eric pointed out he uses a backward compatible idiom in his code.

We would need to make this one safe; something like this, perhaps:

	sub open_read_pipe {
        	my ($fh, @cmd) = @_;
                my $pid = open($fh, '-|');
		die "$!" unless defined($pid);
		if (!$pid) {
			exec(@cmd) or die "$!";
		}
		return $fh;
	}
	...
        open_read_pipe(\*P, qw(git-rev-list --parents --remove-empty),
			$rev, '--', $file);

> +	open(P,"-|","git-diff-tree", "-M50", "-r","--name-status", "-z","$rev")
> +		or die "Failed to exec git-diff: $!";

If you do not mean "I want -M50" but you meant "I want whatever is default", I'd leave that 50 out.

It is probably premature to talk about issues for UI that can be built on top of this, but what I found interesting was this. While looking at the annotate output, whenever I got curious about one line ("Oh, this is an ancient change and by somebody who does not feed patches to git regularly -- what was the change about???"), I grabbed the SHA1 of the commit on the line and threw it at "git show". It was a good way to see how the change to the line was done in context. Maybe a two-pane UI that shows annotate on the top pane, and activating one line from it shows "git show" output on the bottom pane to show the commit log plus changes the commit introduced to the file and other files.