[PATCH] Enable tree (directory) history display

[PATCH] Enable tree (directory) history display

[Luben Tuikov]

This patch allows history display of whole trees/directories a la
"git-rev-list HEAD -- <dir or file>".  I find this useful especially
when a project lives in its own subdirectory, as opposed to being all
of the GIT repository (i.e. when a sub-project is merged into a
super-project).

Signed-off-by: Luben Tuikov <ltuikov@yahoo.com>
---
 gitweb/gitweb.cgi |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/gitweb/gitweb.cgi b/gitweb/gitweb.cgi
index 2705a93..d808900 100755
--- a/gitweb/gitweb.cgi
+++ b/gitweb/gitweb.cgi
@@ -1676,6 +1676,7 @@ #			      " | " . $cgi->a({-href => "$my
 			      "</td>\n" .
 			      "<td class=\"link\">" .
 			      $cgi->a({-href => "$my_uri?" .
esc_param("p=$project;a=tree;h=$t_hash$base_key;f=$base$t_name")}, "tree") .
+			      " | " . $cgi->a({-href => "$my_uri?" .
esc_param("p=$project;a=history;h=$hash_base;f=$base$t_name")}, "history") .
 			      "</td>\n";
 		}
 		print "</tr>\n";
-- 
1.4.1.rc2.g4ce4

Re: [PATCH] Enable tree (directory) history display

[Junio C Hamano]

Luben Tuikov <ltuikov@yahoo.com> writes:

> This patch allows history display of whole trees/directories a la
> "git-rev-list HEAD -- <dir or file>".  I find this useful especially
> when a project lives in its own subdirectory, as opposed to being all
> of the GIT repository (i.e. when a sub-project is merged into a
> super-project).

Both patches from you were seriously damaged.  Check your MUA
before sending further patches, please.

They were trivial for me to apply by hand, so no need to resend
them.  I like the effect of this one I am replying to very much.

Re: [PATCH] Enable tree (directory) history display

[Luben Tuikov]

--- Junio C Hamano <junkio@cox.net> wrote:
> Luben Tuikov <ltuikov@yahoo.com> writes:
> 
> > This patch allows history display of whole trees/directories a la
> > "git-rev-list HEAD -- <dir or file>".  I find this useful especially
> > when a project lives in its own subdirectory, as opposed to being all
> > of the GIT repository (i.e. when a sub-project is merged into a
> > super-project).
> 
> Both patches from you were seriously damaged.  Check your MUA
> before sending further patches, please.

Yes, I'll take a look.

> 
> They were trivial for me to apply by hand, so no need to resend
> them.  I like the effect of this one I am replying to very much.

Junio, Linus,

I took a comparative look with and without "--full-history",
and FWIW, enabling full history just clobbers the output with a lot
of unnecessary information.  I.e. it shows merges which do not have
direct consequence or change to the files in the path spec specified
after the "--".

I.e. no new, relevant information is being shown when "--full-history"
is enabled.  In fact the default git-rev-list case, simplify_history=1,
still shows a merge here and there which doesn't have any direct
changes into what is being sought, but the difference is
about 48% less clobber.

Can you consider the default case to be simplify_history=1,
which is currently the default behaviour of git-rev-list.

If not, is it possible to make this a configurable flag
in gitweb, so that people can decide how much non-related
history to show?  I.e. I'd opt for the default case,
no full-history.

FWIW, I think that the original intention (had there been a choice)
would've been to show only most relevant history, i.e. changes
directly related to paths and files after the "--".

Thanks,
  Luben

Re: [PATCH] Enable tree (directory) history display

[Junio C Hamano]

Luben Tuikov <ltuikov@yahoo.com> writes:

> I took a comparative look with and without "--full-history",
> and FWIW, enabling full history just clobbers the output with a lot
> of unnecessary information.  I.e. it shows merges which do not have
> direct consequence or change to the files in the path spec specified
> after the "--".
>...
> FWIW, I think that the original intention (had there been a choice)
> would've been to show only most relevant history, i.e. changes
> directly related to paths and files after the "--".

Sounds sane.  Maybe we should clearly explain this behaviour
change in the commit log and claim it as an improvement.  We
might even want to do --no-merges if we go that route.

Re: [PATCH] Enable tree (directory) history display

[Luben Tuikov]

--- Junio C Hamano <junkio@cox.net> wrote:
> Luben Tuikov <ltuikov@yahoo.com> writes:
> 
> > I took a comparative look with and without "--full-history",
> > and FWIW, enabling full history just clobbers the output with a lot
> > of unnecessary information.  I.e. it shows merges which do not have
> > direct consequence or change to the files in the path spec specified
> > after the "--".
> >...
> > FWIW, I think that the original intention (had there been a choice)
> > would've been to show only most relevant history, i.e. changes
> > directly related to paths and files after the "--".
> 
> Sounds sane.  Maybe we should clearly explain this behaviour
> change in the commit log and claim it as an improvement.  We
> might even want to do --no-merges if we go that route.

I agree.  Indeed, this would definitely be an improvement.

   Luben

Re: [PATCH] Enable tree (directory) history display

[Linus Torvalds]

On Fri, 30 Jun 2006, Luben Tuikov wrote:
> 
> I took a comparative look with and without "--full-history",
> and FWIW, enabling full history just clobbers the output with a lot
> of unnecessary information.  I.e. it shows merges which do not have
> direct consequence or change to the files in the path spec specified
> after the "--".

Oh, you're right - "--full-history" right now is literally geared solely 
towards the graphical kind, which needs all the merges (regardless of 
whether they change a file or not) to make sense of the history.

> I.e. no new, relevant information is being shown when "--full-history"
> is enabled.  In fact the default git-rev-list case, simplify_history=1,
> still shows a merge here and there which doesn't have any direct
> changes into what is being sought, but the difference is
> about 48% less clobber.

Well, with history simplification, we still show merges that are required 
to make the history _complete_, ie say that you had

	  a
	  |
	  b
	 / \
	c   d
	|   |

and neither "a" nor "b" actually changed the file, but both "c" and "d" 
did: in this case we have to leave "b" around just because otherwise there 
would be no way to show the _relationship_, even if "b" itself doesn't 
actually change the tree in any way what-so-ever.

> Can you consider the default case to be simplify_history=1,
> which is currently the default behaviour of git-rev-list.

Actually, for your case, you don't want _any_ merges, unless those merges 
literally changed the tree from all of the parents.

I think it would make sense to make that further simplification if the 
"--parents" flag wasn't present. 

Hmm. Maybe something like this..

BTW! Junio, I think this patch actually fixes a real bug.

Without this patch, the "--parents --full-history" combination (which 
you'd get if you do something like

	gitk --full-history Makefile

or similar) will actually _drop_ merges where all children are identical. 
That's wrong in the --full-history case, because it measn that the graph 
ends up missing lots of entries.

In the process, this also should make

	git-rev-list --full-history Makefile

give just the _true_ list of all commits that changed Makefile (and 
properly ignore merges that were identical in one parent), because now 
we're not asking for "--parent", so we don't need the unnecessary merge 
commits to keep the history together.

Luben, does this fix the problem for you?

		Linus

----
diff --git a/revision.c b/revision.c
index b963f2a..1cf6276 100644
--- a/revision.c
+++ b/revision.c
@@ -280,7 +280,7 @@ int rev_same_tree_as_empty(struct rev_in
 static void try_to_simplify_commit(struct rev_info *revs, struct commit *commit)
 {
 	struct commit_list **pp, *parent;
-	int tree_changed = 0;
+	int tree_changed = 0, tree_same = 0;
 
 	if (!commit->tree)
 		return;
@@ -298,6 +298,7 @@ static void try_to_simplify_commit(struc
 		parse_commit(p);
 		switch (rev_compare_tree(revs, p->tree, commit->tree)) {
 		case REV_TREE_SAME:
+			tree_same = 1;
 			if (!revs->simplify_history || (p->object.flags & UNINTERESTING)) {
 				/* Even if a merge with an uninteresting
 				 * side branch brought the entire change
@@ -334,7 +335,7 @@ static void try_to_simplify_commit(struc
 		}
 		die("bad tree compare for commit %s", sha1_to_hex(commit->object.sha1));
 	}
-	if (tree_changed)
+	if (tree_changed && !tree_same)
 		commit->object.flags |= TREECHANGE;
 }
 
@@ -896,6 +897,8 @@ static int rewrite_one(struct rev_info *
 		struct commit *p = *pp;
 		if (!revs->limited)
 			add_parents_to_list(revs, p, &revs->commits);
+		if (p->parents && p->parents->next)
+			return 0;
 		if (p->object.flags & (TREECHANGE | UNINTERESTING))
 			return 0;
 		if (!p->parents)
@@ -988,8 +991,15 @@ struct commit *get_revision(struct rev_i
 		    commit->parents && commit->parents->next)
 			continue;
 		if (revs->prune_fn && revs->dense) {
-			if (!(commit->object.flags & TREECHANGE))
-				continue;
+			/* Commit without changes? */
+			if (!(commit->object.flags & TREECHANGE)) {
+				/* drop merges unless we want parenthood */
+				if (!revs->parents)
+					continue;
+				/* non-merge - always ignore it */
+				if (commit->parents && !commit->parents->next)
+					continue;
+			}
 			if (revs->parents)
 				rewrite_parents(revs, commit);
 		}

Re: [PATCH] Enable tree (directory) history display

[Linus Torvalds]

On Fri, 30 Jun 2006, Linus Torvalds wrote:
> 
> BTW! Junio, I think this patch actually fixes a real bug.
> 
> Without this patch, the "--parents --full-history" combination (which 
> you'd get if you do something like
> 
> 	gitk --full-history Makefile
> 
> or similar) will actually _drop_ merges where all children are identical. 
> That's wrong in the --full-history case, because it measn that the graph 
> ends up missing lots of entries.

Here's some real numbers.

Before this patch

	git-rev-list --full-history HEAD -- Makefile | wc -l
	git-rev-list --parents --full-history HEAD -- Makefile | wc -l

both returned 971 commits on my current kernel tree, while

	git-rev-list --parents HEAD -- Makefile | wc -l

returned 145 commits, and

	git-rev-list --parents --no-merges HEAD -- Makefile | wc -l

returns 136.

That count of 145 is the number of commits that actually _change_ Makefile 
some way - and some of them really are merges, because they have a content 
merge, and the merge result is thus different from any of the children. So 
that's a real number. So is 136, in some sense - it just says that we 
don't care about commits, even if those commits _do_ end up changing the 
file.

But the important part to realize is that the "971" number is always 
wrong. It's never a really valid number. It contains a lot of extra 
merges, but it does _not_ contain enough of them to connect all the dots, 
and it's thus never correct. Either you should drop merges that don't 
change things (in which case you cannot have full connectivity, and 
"--parents" doesn't make sense), or you should keep them all (or at least 
enough to get full connectivity).

Now, AFTER this patch

	git-rev-list --full-history HEAD -- Makefile | wc -l

returns 145 commits (the same 145 commits that really change Makefile, but 
we've now properly decided that because we don't have "--parents" and 
don't need to keep connectivity we drop _all_ of the merges that have a 
child that is identical), while

	git-rev-list --parents --full-history HEAD -- Makefile | wc -l

returns 2323 commits, and now really has _all_ the merges (because it 
needs to include every single merge in the tree - otherwise the 
connectivity doesn't make sense).

Now, that 2323 is a bit unnecessary - we end up having merges to merges 
that don't actually have any changes at all in between, and it might be 
nice to simplify the merge history to create a minimal tree that still has 
all potential changes in it, but that's a much harder problem.

Anyway, the patch definitely makes a difference, and I think it's correct. 
The effects might be a bit easier to visualize on a smaller tree than the 
kernel ;)

We could still potentially improve on the "--parents --full-history" case, 
but --full-history currently means always walking all possible chains, and 
that will be shown in the output (ie we will have all possible paths in 
the result if "--parents" is used, even if those paths end up being 
totally uninteresting)

  "Hey - you asked for full history, you got it. Don't blame me if you got 
   a lot of totally uninteresting crud"

				Linus

Re: [PATCH] Enable tree (directory) history display

[Luben Tuikov]

--- Linus Torvalds <torvalds@osdl.org> wrote:
> That count of 145 is the number of commits that actually _change_ Makefile 
> some way - and some of them really are merges, because they have a content 
> merge, and the merge result is thus different from any of the children. So 
> that's a real number. So is 136, in some sense - it just says that we 
> don't care about commits, even if those commits _do_ end up changing the 
> file.

Indeed, I got the same conclusion using different branches.
(Sorry, I thought your email was from Junio in my last email, it was
your analysis after all.)

> But the important part to realize is that the "971" number is always 
> wrong. It's never a really valid number. It contains a lot of extra 
> merges, but it does _not_ contain enough of them to connect all the dots, 
> and it's thus never correct. Either you should drop merges that don't 
> change things (in which case you cannot have full connectivity, and 
> "--parents" doesn't make sense), or you should keep them all (or at least 
> enough to get full connectivity).

Yes, that makes sense.

Ok, I get similar numbers as you do.  After the patch, compared to
the simple case of git-rev-list HEAD -- <path spec>, --parents gives
me only one more commit which is the very first hand made kernel
commit ;-) ; --parents --full-history gives me way too many unrelated
commits about 10x as many; --full-history gives me a _complete_ list.

The simple case fails to report a few commits which are
due to merges, which do indeed change files in the path spec.
(--full-history successfully reported those though).

So your patch plus "--full-history" is what I think we want.

    Luben

Re: [PATCH] Enable tree (directory) history display

[Luben Tuikov]

--- Linus Torvalds <torvalds@osdl.org> wrote:
> Well, with history simplification, we still show merges that are required 
> to make the history _complete_, ie say that you had
> 
> 	  a
> 	  |
> 	  b
> 	 / \
> 	c   d
> 	|   |
> 
> and neither "a" nor "b" actually changed the file, but both "c" and "d" 
> did: in this case we have to leave "b" around just because otherwise there 
> would be no way to show the _relationship_, even if "b" itself doesn't 
> actually change the tree in any way what-so-ever.

I agree.  If "c" and/or "d" changed the file but neither "a" nor "b" did,
then by (merge/diff/etc) "inheritance" we do need to leave "b" around.

(This is similar to git-blame/git-annotate which should show "b", so that
we can track down the change to "c" and/or "d".)

> > Can you consider the default case to be simplify_history=1,
> > which is currently the default behaviour of git-rev-list.
> 
> Actually, for your case, you don't want _any_ merges, unless those merges 
> literally changed the tree from all of the parents.

Yes, that's true.  s/all/one or more:
Don't want to show a merge, unless one or more of the parents,
changed the file.  If no parent changed the tree, then do not
show the commit.

> I think it would make sense to make that further simplification if the 
> "--parents" flag wasn't present. 
> 
> Hmm. Maybe something like this..
> 
> BTW! Junio, I think this patch actually fixes a real bug.
> 
> Without this patch, the "--parents --full-history" combination (which 
> you'd get if you do something like
> 
> 	gitk --full-history Makefile
> 
> or similar) will actually _drop_ merges where all children are identical. 
> That's wrong in the --full-history case, because it measn that the graph 
> ends up missing lots of entries.
> 
> In the process, this also should make
> 
> 	git-rev-list --full-history Makefile
> 
> give just the _true_ list of all commits that changed Makefile (and 
> properly ignore merges that were identical in one parent), because now 
> we're not asking for "--parent", so we don't need the unnecessary merge 
> commits to keep the history together.
> 
> Luben, does this fix the problem for you?

Given Junio's analysis, and briefly looking at the logic, it does seem
correct.  Let me apply it and see what I get, but I think it is a good thing.

Thanks for the patch!

      Luben