threads / discuss / 13792

log --graph --first-parent weirdness

Subject: log --graph --first-parent weirdness

## tl;dr

11 messages between Jun 4, 2008 and Jun 5, 2008.

replies: 10people: 4as markdown or json

Teemu Likonen· Jun 4, 2008, 15:00 UTC · lore

The output of "git log --graph --first-parent" seems weird. Maybe it's because I don't understand everything but here's an example anyway:

$ git log --graph --pretty=oneline --abbrev-commit --decorate
M   2ba1dba... (refs/heads/master) Merge branch 'topic'
|\  
| * 432e062... (refs/heads/topic) Change from branch 'topic'
* | b762236... Change from branch 'master'
|/  
* d7fb80a... Initial commit
Normal --first-parent prints this:
$ git log --first-parent --pretty=oneline --abbrev-commit

2ba1dba... Merge branch 'topic' b762236... Change from branch 'master' d7fb80a... Initial commit

With --graph it goes:
$ git log --graph --first-parent --pretty=oneline --abbrev-commit
M   2ba1dba... Merge branch 'topic'
|\  
* | b762236... Change from branch 'master'
* | d7fb80a... Initial commit
 /

So, it prints the second parent line but it leads to nowhere. Try the same with the git repository and you'll see a _lots_ of parallel branch lines which seem to go nowhere.

Teemu Likonen· Jun 4, 2008, 15:08 UTC · re: Teemu Likonen · lore

Re: log --graph --first-parent weirdness

Teemu Likonen wrote (2008-06-04 18:00 +0300):
Show 12 quoted lines
> $ git log --graph --first-parent --pretty=oneline --abbrev-commit
> 
> M   2ba1dba... Merge branch 'topic'
> |\  
> * | b762236... Change from branch 'master'
> * | d7fb80a... Initial commit
>  /
> 
> 
> So, it prints the second parent line but it leads to nowhere. Try the
> same with the git repository and you'll see a _lots_ of parallel
> branch lines which seem to go nowhere.

And by the way, I'm pretty sure that some earlier log --graph version printed only this with --first-parent:

M   2ba1dba... Merge branch 'topic'
|\  
*   b762236... Change from branch 'master'
*   d7fb80a... Initial commit
Junio C Hamano· Jun 4, 2008, 17:12 UTC · re: Teemu Likonen · lore

Re: log --graph --first-parent weirdness

Teemu Likonen <tlikonen@iki.fi> writes:
> The output of "git log --graph --first-parent" seems weird.

Heh, --first-parent means "I'll view everything as a single strand of pearls". Who in the right mind would use --graph at the same time to begin with ;-)?

We could turn --graph automatically off if --first-parent is given, but I tend to agree with you that the right behaviour is to show the same "everything prefixed with '| ', wasting two columns without good reason" output as you would see on a true linear history.

Teemu Likonen· Jun 4, 2008, 17:38 UTC · re: Junio C Hamano · lore

Re: log --graph --first-parent weirdness

Junio C Hamano wrote (2008-06-04 10:12 -0700):
Show 7 quoted lines
> Teemu Likonen <tlikonen@iki.fi> writes:
> 
> > The output of "git log --graph --first-parent" seems weird.
> 
> Heh, --first-parent means "I'll view everything as a single strand of
> pearls".  Who in the right mind would use --graph at the same time to
> begin with ;-)?

Exactly :) I have an alias "lg = log --graph" and I almost always use that instead of the normal log. Then I accidentally noticed that my "git lg" doesn't quite fit with --first-parent.

> We could turn --graph automatically off if --first-parent is given,
> but I tend to agree with you that the right behaviour is to show the
> same "everything prefixed with '| ', wasting two columns without good
> reason" output as you would see on a true linear history.

To me it's perfectly fine to turn off --graph when used with --first-parent, but yes, generally users might expect to see a line of M's, *'s and |'s there. At least it would clearly show which commits are merges and which are not.

Adam Simpkins· Jun 4, 2008, 18:04 UTC · re: Teemu Likonen · lore

Re: log --graph --first-parent weirdness

On Wed, Jun 04, 2008 at 08:38:20PM +0300, Teemu Likonen wrote:
Show 17 quoted lines
> Junio C Hamano wrote (2008-06-04 10:12 -0700):
> 
> > Teemu Likonen <tlikonen@iki.fi> writes:
> > 
> > > The output of "git log --graph --first-parent" seems weird.
> 
> <snip>
> 
> > We could turn --graph automatically off if --first-parent is given,
> > but I tend to agree with you that the right behaviour is to show the
> > same "everything prefixed with '| ', wasting two columns without good
> > reason" output as you would see on a true linear history.
> 
> To me it's perfectly fine to turn off --graph when used with
> --first-parent, but yes, generally users might expect to see a line of
> M's, *'s and |'s there. At least it would clearly show which commits are
> merges and which are not.

It should be pretty simple to fix this as suggested. There are two places in graph.c where we loop over the current commit's parents. Changing those to break out after the first commit when revs->first_parent_only is set should result in the desired behavior.

I'll try to get some time this evening or tomorrow to create a patch.
-- 
Adam Simpkins
adam@adamsimpkins.net
Adam Simpkins· Jun 5, 2008, 08:56 UTC · re: Adam Simpkins · lore

[PATCH] graph API: fix "git log --graph --first-parent"

This change teaches the graph API that only the first parent of each commit is interesting when "--first-parent" was specified.

This change also consolidates the graph parent walking logic into two new internal functions, first_interesting_parent() and next_interesting_parent(). A simpler fix would have been to simply break at the end of the 2 existing for loops when graph->revs->first_parent_only is set. However, this change seems nicer, especially if we ever need to add any new loops over the parent list in the future.

Signed-off-by: Adam Simpkins <adam@adamsimpkins.net>
---
 graph.c |   64 ++++++++++++++++++++++++++++++++++++++++++++++++++++----------
 1 files changed, 53 insertions(+), 11 deletions(-)
diff --git a/graph.c b/graph.c
index edfab2d..283b137 100644
--- a/graph.c
+++ b/graph.c
@@ -237,6 +237,52 @@ static int graph_is_interesting(struct git_graph *graph, struct commit *commit)
 	return (commit->object.flags & (UNINTERESTING | TREESAME)) ? 0 : 1;
 }
 
+static struct commit_list *next_interesting_parent(struct git_graph *graph,
+						   struct commit_list *orig)
+{
+	struct commit_list *list;
+
+	/*
+	 * If revs->first_parent_only is set, only the first
+	 * parent is interesting.  None of the others are.
+	 */
+	if (graph->revs->first_parent_only)
+		return NULL;
+
+	/*
+	 * Return the next interesting commit after orig
+	 */
+	for (list = orig->next; list; list = list->next) {
+		if (graph_is_interesting(graph, list->item))
+			return list;
+	}
+
+	return NULL;
+}
+
+static struct commit_list *first_interesting_parent(struct git_graph *graph)
+{
+	struct commit_list *parents = graph->commit->parents;
+
+	/*
+	 * If this commit has no parents, ignore it
+	 */
+	if (!parents)
+		return NULL;
+
+	/*
+	 * If the first parent is interesting, return it
+	 */
+	if (graph_is_interesting(graph, parents->item))
+		return parents;
+
+	/*
+	 * Otherwise, call next_interesting_parent() to get
+	 * the next interesting parent
+	 */
+	return next_interesting_parent(graph, parents);
+}
+
 static void graph_insert_into_new_columns(struct git_graph *graph,
 					  struct commit *commit,
 					  int *mapping_index)
@@ -244,12 +290,6 @@ static void graph_insert_into_new_columns(struct git_graph *graph,
 	int i;
 
 	/*
-	 * Ignore uinteresting commits
-	 */
-	if (!graph_is_interesting(graph, commit))
-		return;
-
-	/*
 	 * If the commit is already in the new_columns list, we don't need to
 	 * add it.  Just update the mapping correctly.
 	 */
@@ -373,9 +413,9 @@ static void graph_update_columns(struct git_graph *graph)
 			int old_mapping_idx = mapping_idx;
 			seen_this = 1;
 			graph->commit_index = i;
-			for (parent = graph->commit->parents;
+			for (parent = first_interesting_parent(graph);
 			     parent;
-			     parent = parent->next) {
+			     parent = next_interesting_parent(graph, parent)) {
 				graph_insert_into_new_columns(graph,
 							      parent->item,
 							      &mapping_idx);
@@ -420,9 +460,11 @@ void graph_update(struct git_graph *graph, struct commit *commit)
 	 * Count how many interesting parents this commit has
 	 */
 	graph->num_parents = 0;
-	for (parent = commit->parents; parent; parent = parent->next) {
-		if (graph_is_interesting(graph, parent->item))
-			graph->num_parents++;
+	for (parent = first_interesting_parent(graph);
+	     parent;
+	     parent = next_interesting_parent(graph, parent))
+	{
+		graph->num_parents++;
 	}
 
 	/*
-- 
1.5.6.rc1.13.g14be6
Junio C Hamano· Jun 4, 2008, 18:05 UTC · re: Teemu Likonen · lore

Re: log --graph --first-parent weirdness

Teemu Likonen <tlikonen@iki.fi> writes:
> To me it's perfectly fine to turn off --graph when used with
> --first-parent, but yes, generally users might expect to see a line of
> M's, *'s and |'s there. At least it would clearly show which commits are
> merges and which are not.

I disagree. If you are doing --first-parent, you do not _care_ what is merge and what is not. Besides, you can easily see that from the log message if you cared enough.

And if the graph actually draws the real ancestry graph (i.e. without --first-parent), the lines visually show which is merge and which is not, so the "M" gets very distracting.

I'd really suggest changing the "M" and use "*" everywhere.
Ping Yin· Jun 5, 2008, 01:37 UTC · re: Junio C Hamano · lore

Re: log --graph --first-parent weirdness

* Junio C Hamano <gitster@pobox.com> [2008-06-04 11:05:38 -0700]:
Show 5 quoted lines
> And if the graph actually draws the real ancestry graph (i.e. without
> --first-parent), the lines visually show which is merge and which is not,
> so the "M" gets very distracting.
> 
> I'd really suggest changing the "M" and use "*" everywhere.
I vote for this change.
Adam Simpkins· Jun 5, 2008, 09:28 UTC · re: Junio C Hamano · lore

Re: log --graph --first-parent weirdness

On Wed, Jun 04, 2008 at 11:05:38AM -0700, Junio C Hamano wrote:
> 
> I'd really suggest changing the "M" and use "*" everywhere.
That's fine with me.  Here's a simple patch to change the behavior.

-- >8 -- "git log --graph": print '*' for all commits, including merges

Previously, merge commits were printed with 'M' instead of '*'. This had the potential to confuse users when not all parents of the merge commit were included in the log output.

As Junio has pointed out, merge commits can almost always be easily identified from the log message, anyway.

Signed-off-by: Adam Simpkins <adam@adamsimpkins.net>
---
 graph.c |   14 --------------
 1 files changed, 0 insertions(+), 14 deletions(-)
diff --git a/graph.c b/graph.c
index edfab2d..c50adcd 100644
--- a/graph.c
+++ b/graph.c
@@ -638,20 +638,6 @@ static void graph_output_commit_char(struct git_graph *graph, struct strbuf *sb)
 	}
 
 	/*
-	 * Print 'M' for merge commits
-	 *
-	 * Note that we don't check graph->num_parents to determine if the
-	 * commit is a merge, since that only tracks the number of
-	 * "interesting" parents.  We want to print 'M' for merge commits
-	 * even if they have less than 2 interesting parents.
-	 */
-	if (graph->commit->parents != NULL &&
-	    graph->commit->parents->next != NULL) {
-		strbuf_addch(sb, 'M');
-		return;
-	}
-
-	/*
 	 * Print '*' in all other cases
 	 */
 	strbuf_addch(sb, '*');
-- 
1.5.6.rc1.13.g14be6
Teemu Likonen· Jun 5, 2008, 09:50 UTC · re: Junio C Hamano · lore

Re: log --graph --first-parent weirdness

Junio C Hamano wrote (2008-06-04 11:05 -0700):
Show 16 quoted lines
> Teemu Likonen <tlikonen@iki.fi> writes:
> 
> > To me it's perfectly fine to turn off --graph when used with
> > --first-parent, but yes, generally users might expect to see a line
> > of M's, *'s and |'s there. At least it would clearly show which
> > commits are merges and which are not.
> 
> I disagree.  If you are doing --first-parent, you do not _care_ what
> is merge and what is not.  Besides, you can easily see that from the
> log message if you cared enough.
> 
> And if the graph actually draws the real ancestry graph (i.e. without
> --first-parent), the lines visually show which is merge and which is
> not, so the "M" gets very distracting.
> 
> I'd really suggest changing the "M" and use "*" everywhere.

Well, I disagree :-) Merges are interesting points in history (they introduce features etc.) and for a "--graph --first-parent" user a certain already known merge is easier to find if there is a stable identifier for them (like "M"). Commit messages are not stable in that sense and it helps if user can just keep an eye on the graph when searching for a certain merge (helps to skip other commits). Once the correct merge is found, one would perhaps do "git log -p <the-SHA1-of-that-merge>^2".

So I like having separate identifiers for merge commits. However, I do realize that in the bigger picture those M's are not at all essential for finding wanted information from project's history. So this question is not something I'd go arguing too seriously.

Junio C Hamano· Jun 5, 2008, 18:31 UTC · re: Teemu Likonen · lore

Re: log --graph --first-parent weirdness

Teemu Likonen <tlikonen@iki.fi> writes:
> Well, I disagree :-) Merges are interesting points in history (they
> introduce features etc.) and for a "--graph --first-parent" user
> a certain already known merge is easier to find if there is a stable
> identifier for them.

Step back a bit. Regular commits also introduce features. If you want to argue for marking a merge as more significant than single parent commits, you need to justify the reason why a bit better.

When you are looking at a history (be it 'first-parent' or regular), each transition introduces changes, but especially when you are talking about first-parent, a merge is merely a squashed commit of everything that happened on the side branch, which may be trivial one-liner fix or an addition of full new command. Why a merge of trivial one-liner fix should be treated as more significant than a more involved change that directly was done on the master branch?

A full and perfect implementation of a new command may have happened on a side branch as a single commit. If the master branch was dormant while it was being done, the final merge of that side branch will result in a fast-forward, and the introduction of the new command would appear as a non-merge, regular commit. If on the other hand there were activities on master since the side branch forked, the introduction of the new command would appear as a merge. Why do you paint the latter as more significant than the former?

If somebody argues for making the marking different (perhaps by color-code the asterisk differently) depending on how much each commit changes the tree relative to its parents, I would say it might be a great feature. Such a display would treat the two cases I mentioned above equally.

I however do not think the number of recorded parents deserves such a special treatment to clutter the output and distract people, especially when "is it a merge?" can be easily seen by two other means (log message and graph lines).

← back to recent threads