log --graph --first-parent weirdness

log --graph --first-parent weirdness

[Teemu Likonen]

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.

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

[Teemu Likonen]

Teemu Likonen wrote (2008-06-04 18:00 +0300):

> $ 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

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

[Junio C Hamano]

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.

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

[Teemu Likonen]

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.
> 
> 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.

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

[Adam Simpkins]

On Wed, Jun 04, 2008 at 08:38:20PM +0300, Teemu Likonen wrote:
> 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

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

[Adam Simpkins]

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

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

[Junio C Hamano]

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.

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

[Ping Yin]

* Junio C Hamano <gitster@pobox.com> [2008-06-04 11:05:38 -0700]:

> 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.

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

[Adam Simpkins]

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

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

[Teemu Likonen]

Junio C Hamano wrote (2008-06-04 11:05 -0700):

> 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.

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

[Junio C Hamano]

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).