git-log --follow?

The message I am following up is a patch to unpack-trees.c,
whose basic code structure is Daniel's work, so I wanted to CC
him and the easiest way to look his address up was to run
git-log on it.

Not so.

The "blame -C unpack-trees.c" output consists of this
distribution of origin:

    464 read-tree.c
    337 unpack-trees.c
     74 builtin-read-tree.c
     11 tree.c
      8 tree.h

and most of the work by Daniel was done when the bulk of code
was still in read-tree.c.  Naturally the log output of
unpack-trees.c does not have a single commit by him.  "git log
-- unpack-trees.c" would not follow into read-tree.c, but I
thought "git log --follow -- unpack-trees.c" is supposed to; I
tried it for the first time, but it does not seem to work as
well as I hoped.

I think this is just a testament that "following renames" is not
as useful in a real project as people seem to believe, not a
real complaint.

When 16da134 created unpack-trees.c, it initially moved only
very small part of builtin-read-tree.c to it.  Later 076b0adc
made further code movements from builtin-read-tree.c to
unpack-trees.c.

An interesting thing is that builtin-read-tree.c immediately
before 16da134 is much similar to unpack-trees.c in 076b0adc
than unpack-trees.c in 16da134, exactly because of this stepwise
code movements.  I do not think people can argue that "human
user knows he is renaming the file so recording the human
intention would have helped git a lot better" in this case, as
the human user who made 16da134 did not even intend to do a
rename.

Hi,

On Thu, 12 Jul 2007, Junio C Hamano wrote:

The message I am following up is a patch to unpack-trees.c, whose basic 
code structure is Daniel's work, so I wanted to CC him and the easiest 
way to look his address up was to run git-log on it.

Not so.

[...]

"git log -- unpack-trees.c" would not follow into read-tree.c, but I 
thought "git log --follow -- unpack-trees.c" is supposed to; I tried it 
for the first time, but it does not seem to work as well as I hoped.

Your lesson as to why following renames is not as useful as some might 
want to make us believe is duly noted; will use it as back reference 
should I have to defend that view again.

However, I have to wonder why you did not solve your problem this way:

	git log --author=Daniel

Ciao,
Dscho

On Thu, 12 Jul 2007, Junio C Hamano wrote:

I think this is just a testament that "following renames" is not
as useful in a real project as people seem to believe, not a
real complaint.

Yeah. That said, what you wanted would have actually worked with my 
original strange patch to "git blame", and in particular that also would 
allow you to get a "log" for certain lines in the file.

So something like

	git blame -C -Lx,y --log

may still make sense to give people.

Here's a new version of that patch (I've long since lost the original one, 
plus it probably wouldn't apply anyway, but it was easy to re-generate).

It doesn't set up the revs thing nicely, so trying to add "-p" or "--stat" 
etc doesn't really get you what you'd want, and "--decorate" doesn't work 
since it doesn't call "cmd_log_init".

So this certainly has some room for improvement, but my point is that "git 
blame" actually knows all this, and in many ways does a better job than 
"git log" (but a very *different* job!!).

So "git log" gives you the log for a (set of) pathname(s), while with this 
patch, "git blame --log" gives you the log for the commits that can be 
*blamed* for the current state of that pathname!

Two very different things, but both are valid, and interesting things to 
do, I think. And I really think it's worth doing, if only because it's so 
simple, and fits so well with the whole "git blame" structure!

			Linus

---

diff --git a/builtin-blame.c b/builtin-blame.c
index 0519339..8de06d3 100644
--- a/builtin-blame.c
+++ b/builtin-blame.c

@@ -18,6 +18,7 @@
 #include "cache-tree.h"
 #include "path-list.h"
 #include "mailmap.h"
+#include "log-tree.h"
 
 static char blame_usage[] =
 "git-blame [-c] [-b] [-l] [--root] [-t] [-f] [-n] [-s] [-p] [-w] [-L n,m] [-S <revs-file>] [-M] [-C] [-C] [--contents <filename>] [--incremental] [commit] [--] file\n"

@@ -34,6 +35,7 @@ static char blame_usage[] =
 "  -L n,m              Process only line range n,m, counting from 1\n"
 "  -M, -C              Find line movements within and across files\n"
 "  --incremental       Show blame entries as we find them, incrementally\n"
+"  --log               Show blame entries as we find them in 'git log' style\n"
 "  --contents file     Use <file>'s contents as the final image\n"
 "  -S revs-file        Use revisions from revs-file instead of calling git-rev-list\n";
 

@@ -45,6 +47,7 @@ static int max_score_digits;
 static int show_root;
 static int blank_boundary;
 static int incremental;
+static int blame_log;
 static int cmd_is_annotate;
 static int xdl_opts = XDF_NEED_MINIMAL;
 static struct path_list mailmap;

@@ -1431,11 +1434,22 @@ static void write_filename_info(const char *path)
  * The blame_entry is found to be guilty for the range.  Mark it
  * as such, and show it in incremental output.
  */
-static void found_guilty_entry(struct blame_entry *ent)
+static void found_guilty_entry(struct blame_entry *ent, struct rev_info *rev)
 {
 	if (ent->guilty)
 		return;
 	ent->guilty = 1;
+	if (blame_log) {
+		struct origin *suspect = ent->suspect;
+		struct commit *commit = suspect->commit;
+
+		if (commit->object.flags & METAINFO_SHOWN)
+			return;
+		commit->object.flags |= METAINFO_SHOWN;
+		log_tree_commit(rev, commit);
+		maybe_flush_or_die(stdout, "stdout");
+		return;
+	}
 	if (incremental) {
 		struct origin *suspect = ent->suspect;
 

@@ -1505,7 +1519,7 @@ static void assign_blame(struct scoreboard *sb, struct rev_info *revs, int opt)
 		/* Take responsibility for the remaining entries */
 		for (ent = sb->ent; ent; ent = ent->next)
 			if (same_suspect(ent->suspect, suspect))
-				found_guilty_entry(ent);
+				found_guilty_entry(ent, revs);
 		origin_decref(suspect);
 
 		if (DEBUG) /* sanity */

@@ -2204,6 +2218,8 @@ int cmd_blame(int argc, const char **argv, const char *prefix)
 		}
 		else if (!strcmp("--incremental", arg))
 			incremental = 1;
+		else if (!strcmp("--log", arg))
+			save_commit_buffer = incremental = blame_log = 1;
 		else if (!strcmp("--score-debug", arg))
 			output_option |= OUTPUT_SHOW_SCORE;
 		else if (!strcmp("-f", arg) ||

@@ -2224,7 +2240,7 @@ int cmd_blame(int argc, const char **argv, const char *prefix)
 			argv[unk++] = arg;
 	}
 
-	if (!incremental)
+	if (blame_log || !incremental)
 		setup_pager();
 
 	if (!blame_move_score)

@@ -2324,7 +2340,14 @@ int cmd_blame(int argc, const char **argv, const char *prefix)
 	argv[unk] = NULL;
 
 	init_revisions(&revs, NULL);
+
+	/* Maybe we should call "cmd_log_init()" here instead? */
+	revs.always_show_header = 1;
+	revs.commit_format = CMIT_FMT_DEFAULT;
+	revs.verbose_header = 1;
+	revs.abbrev = DEFAULT_ABBREV;
 	setup_revisions(unk, argv, &revs, NULL);
+
 	memset(&sb, 0, sizeof(sb));
 
 	/*

Linus Torvalds [off-list ref] writes:

On Thu, 12 Jul 2007, Junio C Hamano wrote:

quoted

I think this is just a testament that "following renames" is not
as useful in a real project as people seem to believe, not a
real complaint.

Yeah. That said, what you wanted would have actually worked with my 
original strange patch to "git blame", and in particular that also would 
allow you to get a "log" for certain lines in the file.

Yeah, I just tried the blame from 'pu' (I have been carrying
that original patch from you there).  The output is not very
intuitive in that it talks about each commit but it is not
apparent _why_ the command talks about that commit.  Maybe
adding "there are the lines in the final image of the blob you
are blaming that came from this commit" to the output would make
the output easier to read.

On Thu, 12 Jul 2007, Junio C Hamano wrote:

Yeah, I just tried the blame from 'pu' (I have been carrying
that original patch from you there).  The output is not very
intuitive in that it talks about each commit but it is not
apparent _why_ the command talks about that commit.  Maybe
adding "there are the lines in the final image of the blob you
are blaming that came from this commit" to the output would make
the output easier to read.

That doesn't necessarily work, at least not incrementally. The same commit 
can show up multiple times for *different* 'blame_entry' things, and I 
don't think we want to show such a commit multiple times, and I also don't 
think we know what all the blame entries are going to be until the end.

So right now I use that METAINFO_SHOWN flag to just show the commit once. 
That makes the log a *lot* more readable.

In my original patch (the one you apparently have in 'pu'), I did that 
differently (and not very well), but maybe that approach lends itself 
better to showing some kind of patch.

I think my newer version is likely better.

		Linus