threads / patch / 4724

patchEnable tree (directory) history display

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

## tl;dr

9 messages between Jul 1, 2006 and Jul 1, 2006. Diffs are folded; open one to read it.

replies: 8people: 3as markdown or json

Luben Tuikov· Jul 1, 2006, 01:00 UTC · lore

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(-)
Show changes to gitweb/gitweb.cgi +1 −0
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
Junio C Hamano· Jul 1, 2006, 02:10 UTC · re: Luben Tuikov · lore

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

Luben Tuikov <ltuikov@yahoo.com> writes:
Show 5 quoted lines
> 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.

Luben Tuikov· Jul 1, 2006, 02:43 UTC · re: Junio C Hamano · lore

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

--- Junio C Hamano <junkio@cox.net> wrote:
Show 10 quoted lines
> 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
Junio C Hamano· Jul 1, 2006, 03:01 UTC · re: Luben Tuikov · lore

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

Luben Tuikov <ltuikov@yahoo.com> writes:
Show 9 quoted lines
> 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.

Luben Tuikov· Jul 1, 2006, 03:10 UTC · re: Junio C Hamano · lore

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

--- Junio C Hamano <junkio@cox.net> wrote:
Show 15 quoted lines
> 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
Linus Torvalds· Jul 1, 2006, 03:21 UTC · re: Luben Tuikov · lore

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

On Fri, 30 Jun 2006, Luben Tuikov wrote:
Show 6 quoted lines
> 
> 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.

Show 5 quoted lines
> 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
----
Show changes to revision.c +14 −4
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);
 		}
Linus Torvalds· Jul 1, 2006, 03:45 UTC · re: Linus Torvalds · lore

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

On Fri, 30 Jun 2006, Linus Torvalds wrote:
Show 11 quoted lines
> 
> 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
Luben Tuikov· Jul 1, 2006, 22:45 UTC · re: Linus Torvalds · lore

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

--- Linus Torvalds <torvalds@osdl.org> wrote:
Show 6 quoted lines
> 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.)

Show 7 quoted lines
> 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
Luben Tuikov· Jul 1, 2006, 19:38 UTC · re: Linus Torvalds · lore

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

--- Linus Torvalds <torvalds@osdl.org> wrote:
Show 14 quoted lines
> 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".)

Show 5 quoted lines
> > 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.

Show 26 quoted lines
> 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

← back to recent threads