Re: merge summaries

Linus Torvalds [off-list ref] writes:

quoted

I have two questions:
1) Why isn't 'true' the default setting? The two probably most prominent git
repositories, git and kernel, both use merge summaries.

Think about _who_ uses it for those two prominent repositories for a while.

[ pause, ponder here ]

In other words, "merge.summary" should be true ONLY FOR TOP-LEVEL 
MAINTAINERS.

Making it true for anybody else is almost always a bug. Yes, I and Junio 
have it enabled, but pretty much nobody else should, because if they pull 
from me or Junio in order to merge, their merge summaries would be exactly 
the wrong way around!

This does not necessarily mean that your lieutenants should not
use merge.summary when they pull from other trees (or inside
their own repository to merge in the topics).  They need to
however disable it with --no-summary when they pull from you
when they choose to merge instead of rebase to adjust to the
updated infrastructure in your tree.

On Fri, 18 May 2007, Junio C Hamano wrote:

This does not necessarily mean that your lieutenants should not
use merge.summary when they pull from other trees (or inside
their own repository to merge in the topics).  They need to
however disable it with --no-summary when they pull from you
when they choose to merge instead of rebase to adjust to the
updated infrastructure in your tree.

Yes, the problem is that people *will* get it wrong, so right now I'd 
discourage people from even trying to enable merge summaries unless they 
are the top-level maintainer.

Of course, we could have some heuristics to make it possibly work well 
enough in practice that we could make it useful to more people:

 - take the "merge.summary" field from the "remote" description for any 
   shorthand cases, always defaulting to "off" (regardless of what the 
   "merge.summary" config value is)

   End result: "git pull origin" would never generate a merge summary, 
   unless somebody explicitly sets

	[remote "origin"]
		mergesummary = true

 - use the "merge.summary" flag only for explicitly named remotes (and 
   possibly add "--summary" so that people can choose to never do it by 
   default, but do it explicitly for when they pull from a sublieutenant)

Hmm?

		Linus

On Fri, May 18, 2007 at 12:13:02PM -0700, Linus Torvalds wrote:

On Fri, 18 May 2007, Junio C Hamano wrote:

quoted

This does not necessarily mean that your lieutenants should not
use merge.summary when they pull from other trees (or inside
their own repository to merge in the topics).  They need to
however disable it with --no-summary when they pull from you
when they choose to merge instead of rebase to adjust to the
updated infrastructure in your tree.

Yes, the problem is that people *will* get it wrong, so right now I'd 
discourage people from even trying to enable merge summaries unless they 
are the top-level maintainer.

I never quite understood what they're for--do they add any information
not already available in the history?  If not, and if people still find
them helpful anyway, then I dunno, it looks like a sign of some sort of
failure of our history display tools.

--b.

On Fri, 18 May 2007, J. Bruce Fields wrote:

I never quite understood what they're for--do they add any information
not already available in the history?  If not, and if people still find
them helpful anyway, then I dunno, it looks like a sign of some sort of
failure of our history display tools.

I don't think they add much in the case of a graphical viewer, but for 
"git log", it does make it look nicer..

		Linus

On May 18, 2007, at 8:47 PM, Junio C Hamano wrote:

quoted

Think about _who_ uses it for those two prominent repositories for  
a while.

[ pause, ponder here ]

In other words, "merge.summary" should be true ONLY FOR TOP-LEVEL
MAINTAINERS.

Making it true for anybody else is almost always a bug. Yes, I and  
Junio
have it enabled, but pretty much nobody else should, because if  
they pull
from me or Junio in order to merge, their merge summaries would be  
exactly
the wrong way around!

This does not necessarily mean that your lieutenants should not
use merge.summary when they pull from other trees (or inside
their own repository to merge in the topics).  They need to
however disable it with --no-summary when they pull from you
when they choose to merge instead of rebase to adjust to the
updated infrastructure in your tree.

[ ... pondering ... ]

Apparently parents of merge commits are not fully equal and summaries
can help to browse the history of a branch. If summaries are enabled
"git log --first-parent" gives hints to all changes at least in
short-log form. Changes along the first-parent-path to the initial
commit are listed in full. All changes that were merged in are
listed in the summaries of the merge. This seems to be a useful feature.

But isn't this true for any branch and any merge. If someone pulls from
you or Junio and runs "git log --first-parent" why shouldn't he see
a summary of the changes that were pulled? At that point his branch's
first-parent-path to the initial commit would not touch the changes
pulled from you. So the summary could help.

What is so wrong if merge summaries of pulls from you were committed
to topic branches. Later when you pull such branches the summaries
shouldn't be a problem. A short-log of the topic branch wouldn't
contain the summaries.

Probably I do not fully understand what "the wrong way around" means.

- Steffen

Summaries in oneline output are indented to show the location of
parents in merge commits. Indentation level is computed as the
smallest sum of parent locations along all paths that reach the
commit.

As a result, the output of
    git-rev-list --pretty=oneline --topo-order --indent
is formatted in a way that resembles merge summaries. All commits that
were pulled from a branch are indented with one additional space below
the summary of the merge commit.

WARNING: this patch changes the binary layout of commit.h. This could
probably be avoided by putting a bit more effort into the
implementation.

Signed-off-by: Steffen Prohaska <redacted>
---
 builtin-rev-list.c |    7 ++++++-
 commit.h           |    1 +
 revision.c         |   19 +++++++++++++++----
 revision.h         |    3 ++-
 4 files changed, 24 insertions(+), 6 deletions(-)

diff --git a/builtin-rev-list.c b/builtin-rev-list.c
index ebf53f5..fd880b0 100644
--- a/builtin-rev-list.c
+++ b/builtin-rev-list.c

@@ -50,6 +50,7 @@ static const char *header_prefix;
 
 static void show_commit(struct commit *commit)
 {
+	int i;
 	if (show_timestamp)
 		printf("%lu ", commit->date);
 	if (header_prefix)

@@ -86,8 +87,12 @@ static void show_commit(struct commit *commit)
 		     parents = parents->next)
 			parents->item->object.flags &= ~TMP_MARK;
 	}
-	if (revs.commit_format == CMIT_FMT_ONELINE)
+	if (revs.commit_format == CMIT_FMT_ONELINE) {
 		putchar(' ');
+		if (revs.indent)
+			for (i = 0; i < commit->level; i++)
+				putchar(' ');
+	}
 	else
 		putchar('\n');
 

diff --git a/commit.h b/commit.h
index 86e8dca..e0e7955 100644
--- a/commit.h
+++ b/commit.h

@@ -17,6 +17,7 @@ struct commit {
 	struct commit_list *parents;
 	struct tree *tree;
 	char *buffer;
+	int level;
 };
 
 extern int save_commit_buffer;

diff --git a/revision.c b/revision.c
index 0125d41..c965a23 100644
--- a/revision.c
+++ b/revision.c

@@ -372,7 +372,7 @@ static int add_parents_to_list(struct rev_info *revs, struct commit *commit, str
 {
 	struct commit_list *parent = commit->parents;
 	unsigned left_flag;
-	int add, rest;
+	int add, rest, indent;
 
 	if (commit->object.flags & ADDED)
 		return 0;

@@ -421,18 +421,24 @@ static int add_parents_to_list(struct rev_info *revs, struct commit *commit, str
 	left_flag = (commit->object.flags & SYMMETRIC_LEFT);
 
 	rest = !revs->first_parent_only;
-	for (parent = commit->parents, add = 1; parent; add = rest) {
+	for (parent = commit->parents, add = 1, indent = 0; parent; add = rest, indent++) {
 		struct commit *p = parent->item;
 
 		parent = parent->next;
 		if (parse_commit(p) < 0)
 			return -1;
 		p->object.flags |= left_flag;
-		if (p->object.flags & SEEN)
+		if (p->object.flags & SEEN) {
+			if (commit->level + indent < p->level) {
+				p->level = commit->level + indent;
+			}
 			continue;
+		}
 		p->object.flags |= SEEN;
-		if (add)
+		if (add) {
+			p->level = commit->level + indent;
 			insert_by_date(p, list);
+		}
 	}
 	return 0;
 }

@@ -1080,6 +1086,10 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, const ch
 				revs->commit_format = get_commit_format(arg+8);
 				continue;
 			}
+			if (!strcmp(arg, "--indent")) {
+				revs->indent = 1;
+				continue;
+			}
 			if (!strcmp(arg, "--root")) {
 				revs->show_root_diff = 1;
 				continue;

@@ -1252,6 +1262,7 @@ int prepare_revision_walk(struct rev_info *revs)
 		if (commit) {
 			if (!(commit->object.flags & SEEN)) {
 				commit->object.flags |= SEEN;
+				commit->level = 0;
 				insert_by_date(commit, &revs->commits);
 			}
 		}

diff --git a/revision.h b/revision.h
index 2845167..2a45955 100644
--- a/revision.h
+++ b/revision.h

@@ -63,7 +63,8 @@ struct rev_info {
 
 	/* Format info */
 	unsigned int	shown_one:1,
-			abbrev_commit:1;
+			abbrev_commit:1,
+			indent:1;
 	enum date_mode date_mode;
 
 	const char **ignore_packed; /* pretend objects in these are unpacked */

-- 
1.5.2.rc3.88.g9f73-dirty

On May 20, 2007, at 6:00 PM, Steffen Prohaska wrote:

As a result, the output of
    git-rev-list --pretty=oneline --topo-order --indent
is formatted in a way that resembles merge summaries. All commits that
were pulled from a branch are indented with one additional space below
the summary of the merge commit.

I created the patch to illustrate how the history could be formatted to
replace summaries.

For example
     git-rev-list --pretty=oneline --topo-order --indent 404fdef2 |  
cut -d ' ' -f 2-
outputs

Merge branch 'maint'
  Documentation: Reformatted SYNOPSIS for several commands
  Documentation: Added [verse] to SYNOPSIS where necessary
  Documentation/git.txt: Update links to older documentation pages.
gitweb: Fix "Use of uninitialized value" warning in git_feed
Merge branch 'sp/cvsexport'
  Optimized cvsexportcommit: calling 'cvs status' once instead of  
once per touched file.
Add link to 1.5.1.5 release notes.
Merge 1.5.1.5 in
  GIT v1.5.1.5
  Merge branch 'maint' of git://linux-nfs.org/~bfields/git into maint
   user-manual: reorganize public git repo discussion
   user-manual: listing commits reachable from some refs not others
   user-manual: introduce git
   user-manual: add a "counting commits" example
   user-manual: move howto/using-topic-branches into manual
   user-manual: move howto/make-dist.txt into user manual
   Documentation: remove howto's now incorporated into manual
   user-manual: move quick-start to an appendix
   glossary: expand and clarify some definitions, prune cross-references
   user-manual: revise birdseye-view chapter
   Add a birdview-on-the-source-code section to the user manual
  Documentation: git-rev-list's "patterns"
gitweb: Remove redundant $searchtype setup
[...]

- Steffen

Steffen Prohaska [off-list ref] writes:

Summaries in oneline output are indented to show the location of
parents in merge commits. Indentation level is computed as the
smallest sum of parent locations along all paths that reach the
commit.

As a result, the output of
    git-rev-list --pretty=oneline --topo-order --indent
is formatted in a way that resembles merge summaries. All commits that
were pulled from a branch are indented with one additional space below
the summary of the merge commit.

I would have expected this kind of UI level tweak to be done not
as a rev-list feature, but the subject of the e-mail to read
something like "--pretty=oneline: allow indentation levels",
i.e. wouldn't the feature apply not just rev-list but also
git-log?

lördag 19 maj 2007 skrev Linus Torvalds:

On Fri, 18 May 2007, J. Bruce Fields wrote:

quoted

I never quite understood what they're for--do they add any information
not already available in the history?  If not, and if people still find
them helpful anyway, then I dunno, it looks like a sign of some sort of
failure of our history display tools.

I don't think they add much in the case of a graphical viewer, but for 
"git log", it does make it look nicer..

It is nice for git-cvsexportcommit too. 

-- robin

On May 20, 2007, at 8:36 PM, Junio C Hamano wrote:

Steffen Prohaska [off-list ref] writes:

quoted

Summaries in oneline output are indented to show the location of
parents in merge commits. Indentation level is computed as the
smallest sum of parent locations along all paths that reach the
commit.

As a result, the output of
    git-rev-list --pretty=oneline --topo-order --indent
is formatted in a way that resembles merge summaries. All commits  
that
were pulled from a branch are indented with one additional space  
below
the summary of the merge commit.

I would have expected this kind of UI level tweak to be done not
as a rev-list feature, but the subject of the e-mail to read
something like "--pretty=oneline: allow indentation levels",
i.e. wouldn't the feature apply not just rev-list but also
git-log?

It's only a proof of concept. I do not expect to see it added in
the current form. I'm not yet convinced if it's really a useful feature.

I'm also not sure if changing 'struct commit' is a good idea.
gitk is able to display the commit graph by parsing output of the
existing git-rev-list. It should be possible to do similar for log
formatting without changing 'struct commit'.

- Steffen