bug in "git log --graph" at the tail end?

bug in "git log --graph" at the tail end?

[Junio C Hamano]

It appears that "log --graph" output has a bug at its tail end.

    $ git log --pretty=oneline --abbrev-commit --graph 9d88058^..1af8bca
    M   1af8bca... Merge branch 'maint'
    |\  
    | M   008442f... Merge branch 'maint-1.5.4' into maint
    | |\  
    | * e77b0b5... git-am: fix typo in usage message
    | * 74190d2... doc/git-daemon: s/uploadarchive/uploadarch/
    M   9d88058... Merge branch 'ar/add-unreadable'
    |\  
    * dad25e4... Add a config option to ignore errors for git-add
    * 8959743... Add a test for git-add --ignore-errors
    * 984b83e... Add --ignore-errors to git-add to allow it to sk...
    * 7ae02a3... Extend interface of add_files_to_cache to allow ...
    * 960b8ad... Make the exit code of add_file_to_index actually...

Ideally, I think the output should look like this:

    M   1af8bca... Merge branch 'maint'
    |\  
    | M   008442f... Merge branch 'maint-1.5.4' into maint
    | |\  
    | * e77b0b5... git-am: fix typo in usage message
    | * 74190d2... doc/git-daemon: s/uploadarchive/uploadarch/
    M   9d88058... Merge branch 'ar/add-unreadable'
    |\  
    | * dad25e4... Add a config option to ignore errors for git-add
    | * 8959743... Add a test for git-add --ignore-errors
    | * 984b83e... Add --ignore-errors to git-add to allow it to sk...
    | * 7ae02a3... Extend interface of add_files_to_cache to allow ...
    | * 960b8ad... Make the exit code of add_file_to_index actually...

This is purely cosmetic, but because --graph is only about the cosmetics,
we may want to do something about it.

Thoughts?

Re: bug in "git log --graph" at the tail end?

[Reece Dunn]

2008/5/23 Junio C Hamano <gitster@pobox.com>:
> It appears that "log --graph" output has a bug at its tail end.
>
>    $ git log --pretty=oneline --abbrev-commit --graph 9d88058^..1af8bca
>    M   1af8bca... Merge branch 'maint'
>    |\
>    | M   008442f... Merge branch 'maint-1.5.4' into maint
>    | |\
>    | * e77b0b5... git-am: fix typo in usage message
>    | * 74190d2... doc/git-daemon: s/uploadarchive/uploadarch/

Shouldn't this be:
>    | M   008442f... Merge branch 'maint-1.5.4' into maint
>    | |\
>    | | * e77b0b5... git-am: fix typo in usage message
>    | | * 74190d2... doc/git-daemon: s/uploadarchive/uploadarch/

as well?

> This is purely cosmetic, but because --graph is only about the cosmetics,
> we may want to do something about it.
>
> Thoughts?

It is also consistent and makes the output easier to scan, so +1.

- Reece

Re: bug in "git log --graph" at the tail end?

[Adam Simpkins]

On Fri, May 23, 2008 at 01:42:56PM -0700, Junio C Hamano wrote:
> It appears that "log --graph" output has a bug at its tail end.
> 
>     $ git log --pretty=oneline --abbrev-commit --graph 9d88058^..1af8bca
>     M   1af8bca... Merge branch 'maint'
>     |\  
>     | M   008442f... Merge branch 'maint-1.5.4' into maint
>     | |\  
>     | * e77b0b5... git-am: fix typo in usage message
>     | * 74190d2... doc/git-daemon: s/uploadarchive/uploadarch/
>     M   9d88058... Merge branch 'ar/add-unreadable'
>     |\  
>     * dad25e4... Add a config option to ignore errors for git-add
>     * 8959743... Add a test for git-add --ignore-errors
>     * 984b83e... Add --ignore-errors to git-add to allow it to sk...
>     * 7ae02a3... Extend interface of add_files_to_cache to allow ...
>     * 960b8ad... Make the exit code of add_file_to_index actually...
> 
> Ideally, I think the output should look like this:
> 
>     M   1af8bca... Merge branch 'maint'
>     |\  
>     | M   008442f... Merge branch 'maint-1.5.4' into maint
>     | |\  
>     | * e77b0b5... git-am: fix typo in usage message
>     | * 74190d2... doc/git-daemon: s/uploadarchive/uploadarch/
>     M   9d88058... Merge branch 'ar/add-unreadable'
>     |\  
>     | * dad25e4... Add a config option to ignore errors for git-add
>     | * 8959743... Add a test for git-add --ignore-errors
>     | * 984b83e... Add --ignore-errors to git-add to allow it to sk...
>     | * 7ae02a3... Extend interface of add_files_to_cache to allow ...
>     | * 960b8ad... Make the exit code of add_file_to_index actually...
> 
> This is purely cosmetic, but because --graph is only about the cosmetics,
> we may want to do something about it.

Yes, you're right.  There are a couple bugs in handling commits with
parents flagged as UNINTERESTING.  There are two problems:

- The code that determines where each branch line should go in the next
  commit completely ignores the uninteresting parent.  Instead, it
  really should use two blank spaces in its computation, to account for
  where the parent would have been.

  This is more than just a cosmetic issue, since it results in branch
  lines being mis-aligned.  This makes the graph show incorrect parent
  child relationships.

  For example, the code should draw this:

  | * | <- commit with uninteresting parent
  |  /
  * |
  | *

  But instead, it currently draws this:

  | * | <- commit with uninteresting parent
  * |
  | *

  This is a simple one-line fix.  I'll submit a patch for it.


- Merge commits that have uninteresting parents print branch lines for
  all of their parents, even the uninteresting ones.

  This is just a cosmetic issue, but I agree it looks bad.  Perhaps the
  best thing to do is not to display anything for the uninteresting
  parents.  It might look a little confusing to some users though, since
  you can have a merge commit that is printed as 'M', but it only has
  one parent displayed in the graph.

  This change would make the output from the command you mentioned
  above look like:

    M   1af8bca... Merge branch 'maint'
    |\  
    | M   008442f... Merge branch 'maint-1.5.4' into maint
    | * e77b0b5... git-am: fix typo in usage message
    | * 74190d2... doc/git-daemon: s/uploadarchive/uploadarch/
    M   9d88058... Merge branch 'ar/add-unreadable'
    * dad25e4... Add a config option to ignore errors for git-add
    * 8959743... Add a test for git-add --ignore-errors
    * 984b83e... Add --ignore-errors to git-add to allow it to sk...
    * 7ae02a3... Extend interface of add_files_to_cache to allow ...
    * 960b8ad... Make the exit code of add_file_to_index actually...

-- 
Adam Simpkins
adam@adamsimpkins.net

Re: bug in "git log --graph" at the tail end?

[Junio C Hamano]

Adam Simpkins <adam@adamsimpkins.net> writes:

> - Merge commits that have uninteresting parents print branch lines for
>   all of their parents, even the uninteresting ones.
>
>   This is just a cosmetic issue, but I agree it looks bad.  Perhaps the
>   best thing to do is not to display anything for the uninteresting
>   parents.  It might look a little confusing to some users though, since
>   you can have a merge commit that is printed as 'M', but it only has
>   one parent displayed in the graph.

Ah, I was also going to suggest you _not_ to use 'M' at all.  In other
words, I do not see anything wrong if you drew the graph like this:

      *   1af8bca... Merge branch 'maint'
     /|
    | *   008442f... Merge branch 'maint-1.5.4' into maint
    | * e77b0b5... git-am: fix typo in usage message
    | * 74190d2... doc/git-daemon: s/uploadarchive/uploadarch/

[PATCH 0/2] fix "log --graph" issues with uninteresting parents

[Adam Simpkins]

This fixes the issues pointed out by Junio when graphing commits that
have parents that aren't printed in the log output.

Adam Simpkins (2):
  graph API: fix graph mis-alignment after uninteresting commits
  graph API: don't print branch lines for uninteresting merge parents

 graph.c |   54 ++++++++++++++++++++++++++++++++++++++++++++----------
 1 files changed, 44 insertions(+), 10 deletions(-)

[PATCH 1/2] graph API: fix graph mis-alignment after uninteresting commits

[Adam Simpkins]

The graphing code had a bug that caused it to output branch lines
incorrectly after ignoring an uninteresting commit.  When computing how
to match up the branch lines from the current commit to the next one, it
forgot to take into account that it needed to initially start with 2
empty spaces where the missing commit would have gone.

So, instead of drawing this,

| * | <- Commit with uninteresting parent
|  /
* |

It used to incorrectly draw this:

| * | <- Commit with uninteresting parent
* |

Signed-off-by: Adam Simpkins <adam@adamsimpkins.net>
---
 graph.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/graph.c b/graph.c
index 9d6ed30..400f014 100644
--- a/graph.c
+++ b/graph.c
@@ -190,7 +190,10 @@ static void graph_insert_into_new_columns(struct git_graph *graph,
 	 * Ignore uinteresting and pruned commits
 	 */
 	if (commit->object.flags & (UNINTERESTING | TREESAME))
+	{
+		*mapping_index += 2;
 		return;
+	}
 
 	/*
 	 * If the commit is already in the new_columns list, we don't need to
-- 
1.5.5.1.359.gebc23.dirty

[PATCH 2/2] graph API: don't print branch lines for uninteresting merge parents

[Adam Simpkins]

Previously, the graphing code printed lines coming out of a merge commit
for all of its parents, even if some of them were uninteresting.  Now it
only prints lines for interesting commits.

For example, for a merge commit where only the first parent is
interesting, the code now prints:

  *  merge commit
  *  interesting child

instead of:

  M  merge commit
  |\
  *  interesting child
---
 graph.c |   57 ++++++++++++++++++++++++++++++++++++++++++++-------------
 1 files changed, 44 insertions(+), 13 deletions(-)

diff --git a/graph.c b/graph.c
index 400f014..add7e44 100644
--- a/graph.c
+++ b/graph.c
@@ -55,9 +55,11 @@ struct git_graph {
 	 */
 	struct commit *commit;
 	/*
-	 * The number of parents this commit has.
-	 * (Stored so we don't have to walk over them each time we need
-	 * this number)
+	 * The number of interesting parents that this commit has.
+	 *
+	 * Note that this is not the same as the actual number of parents.
+	 * This count excludes parents that won't be printed in the graph
+	 * output, as determined by graph_is_interesting().
 	 */
 	int num_parents;
 	/*
@@ -180,6 +182,18 @@ static void graph_ensure_capacity(struct git_graph *graph, int num_columns)
 				      sizeof(int) * 2 * graph->column_capacity);
 }
 
+/*
+ * Returns 1 if the commit will be printed in the graph output,
+ * and 0 otherwise.
+ */
+static int graph_is_interesting(struct commit *commit)
+{
+	/*
+	 * Uninteresting and pruned commits won't be printed
+	 */
+	return (commit->object.flags & (UNINTERESTING | TREESAME)) ? 0 : 1;
+}
+
 static void graph_insert_into_new_columns(struct git_graph *graph,
 					  struct commit *commit,
 					  int *mapping_index)
@@ -187,13 +201,10 @@ static void graph_insert_into_new_columns(struct git_graph *graph,
 	int i;
 
 	/*
-	 * Ignore uinteresting and pruned commits
+	 * Ignore uinteresting commits
 	 */
-	if (commit->object.flags & (UNINTERESTING | TREESAME))
-	{
-		*mapping_index += 2;
+	if (!graph_is_interesting(commit))
 		return;
-	}
 
 	/*
 	 * If the commit is already in the new_columns list, we don't need to
@@ -231,8 +242,8 @@ static void graph_update_width(struct git_graph *graph,
 	int max_cols = graph->num_columns + graph->num_parents;
 
 	/*
-	 * Even if the current commit has no parents, it still takes up a
-	 * column for itself.
+	 * Even if the current commit has no parents to be printed, it
+	 * still takes up a column for itself.
 	 */
 	if (graph->num_parents < 1)
 		max_cols++;
@@ -316,6 +327,7 @@ static void graph_update_columns(struct git_graph *graph)
 		}
 
 		if (col_commit == graph->commit) {
+			int old_mapping_idx = mapping_idx;
 			seen_this = 1;
 			for (parent = graph->commit->parents;
 			     parent;
@@ -324,6 +336,14 @@ static void graph_update_columns(struct git_graph *graph)
 							      parent->item,
 							      &mapping_idx);
 			}
+			/*
+			 * We always need to increment mapping_idx by at
+			 * least 2, even if it has no interesting parents.
+			 * The current commit always takes up at least 2
+			 * spaces.
+			 */
+			if (mapping_idx == old_mapping_idx)
+				mapping_idx += 2;
 		} else {
 			graph_insert_into_new_columns(graph, col_commit,
 						      &mapping_idx);
@@ -353,11 +373,13 @@ void graph_update(struct git_graph *graph, struct commit *commit)
 	graph->commit = commit;
 
 	/*
-	 * Count how many parents this commit has
+	 * Count how many interesting parents this commit has
 	 */
 	graph->num_parents = 0;
-	for (parent = commit->parents; parent; parent = parent->next)
-		graph->num_parents++;
+	for (parent = commit->parents; parent; parent = parent->next) {
+		if (graph_is_interesting(parent->item))
+			graph->num_parents++;
+	}
 
 	/*
 	 * Call graph_update_columns() to update
@@ -543,6 +565,15 @@ void graph_output_commit_line(struct git_graph *graph, struct strbuf *sb)
 
 		if (col_commit == graph->commit) {
 			seen_this = 1;
+			/*
+			 * If the commit has more than 1 interesting
+			 * parent, print 'M' to indicate that it is a
+			 * merge.  Otherwise, print '*'.
+			 *
+			 * Note that even if this is actually a merge
+			 * commit, we still print '*' if less than 2 of its
+			 * parents are interesting.
+			 */
 			if (graph->num_parents > 1)
 				strbuf_addch(sb, 'M');
 			else
-- 
1.5.5.1.359.gebc23.dirty

Re: [PATCH 2/2] graph API: don't print branch lines for uninteresting merge parents

[Teemu Likonen]

Adam Simpkins wrote (2008-05-23 19:24 -0700):

> Previously, the graphing code printed lines coming out of a merge commit
> for all of its parents, even if some of them were uninteresting.  Now it
> only prints lines for interesting commits.
> 
> For example, for a merge commit where only the first parent is
> interesting, the code now prints:
> 
>   *  merge commit
>   *  interesting child
> 
> instead of:
> 
>   M  merge commit
>   |\
>   *  interesting child

I'd rather show merge commits always with M. They are still conceptually
merges even if the current log view shows only one parent. This patch
changes symbols * and M to be visual symbols, not logical symbols which
I'd prefer.

For example user may be interested in finding certain merge but chooses
to limit the output with a commit range. It helps if user can always
trust the symbols * and M to have this conceptual difference.

[PATCH 0/1] graph API: always print 'M' for merge commits

[Adam Simpkins]

On Sat, May 24, 2008 at 09:25:30PM +0300, Teemu Likonen wrote:
> 
> I'd rather show merge commits always with M. They are still conceptually
> merges even if the current log view shows only one parent. This patch
> changes symbols * and M to be visual symbols, not logical symbols which
> I'd prefer.
> 
> For example user may be interested in finding certain merge but chooses
> to limit the output with a commit range. It helps if user can always
> trust the symbols * and M to have this conceptual difference.

Okay.  Here's a simple patch to restore the behavior of always printing
'M' for merge commits, even if less than 2 parents are shown in the
graph.


Adam Simpkins (1):
  graph API: always print 'M' for merge commits

 graph.c |   16 +++++++++-------
 1 files changed, 9 insertions(+), 7 deletions(-)

[PATCH 1/1] graph API: always print 'M' for merge commits

[Adam Simpkins]

The recent changes for handling uninteresting commits changed the
behavior so that merge commits were displayed as 'M' only if they had
multiple interesting parents.

This change reverts to the old behavior of displaying merges as 'M',
even if they have less than 2 parents displayed in the graph.
---
 graph.c |   16 +++++++++-------
 1 files changed, 9 insertions(+), 7 deletions(-)

diff --git a/graph.c b/graph.c
index add7e44..ba9ede0 100644
--- a/graph.c
+++ b/graph.c
@@ -566,15 +566,17 @@ void graph_output_commit_line(struct git_graph *graph, struct strbuf *sb)
 		if (col_commit == graph->commit) {
 			seen_this = 1;
 			/*
-			 * If the commit has more than 1 interesting
-			 * parent, print 'M' to indicate that it is a
-			 * merge.  Otherwise, print '*'.
+			 * If the commit is a merge, print 'M'.  Otherwise,
+			 * print '*'.
 			 *
-			 * Note that even if this is actually a merge
-			 * commit, we still print '*' if less than 2 of its
-			 * parents are interesting.
+			 * 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->num_parents > 1)
+			if (graph->commit->parents != NULL &&
+			    graph->commit->parents->next != NULL)
 				strbuf_addch(sb, 'M');
 			else
 				strbuf_addch(sb, '*');
-- 
1.5.5.1.385.ge74ed

Re: [PATCH 1/1] graph API: always print 'M' for merge commits

[Adam Simpkins]

On Sat, May 24, 2008 at 01:52:01PM -0700, Adam Simpkins wrote:
> The recent changes for handling uninteresting commits changed the
> behavior so that merge commits were displayed as 'M' only if they had
> multiple interesting parents.
> 
> This change reverts to the old behavior of displaying merges as 'M',
> even if they have less than 2 parents displayed in the graph.

Signed-off-by: Adam Simpkins <adam@adamsimpkins.net>

-- 
Adam Simpkins
adam@adamsimpkins.net

Re: [PATCH 0/1] graph API: always print 'M' for merge commits

[Junio C Hamano]

Adam Simpkins <adam@adamsimpkins.net> writes:

> On Sat, May 24, 2008 at 09:25:30PM +0300, Teemu Likonen wrote:
>> 
>> I'd rather show merge commits always with M. They are still conceptually
>> merges even if the current log view shows only one parent. This patch
>> changes symbols * and M to be visual symbols, not logical symbols which
>> I'd prefer.
>> 
>> For example user may be interested in finding certain merge but chooses
>> to limit the output with a commit range. It helps if user can always
>> trust the symbols * and M to have this conceptual difference.
>
> Okay.  Here's a simple patch to restore the behavior of always printing
> 'M' for merge commits, even if less than 2 parents are shown in the
> graph.

I have to wonder if that is really necessary.  A merge commit, unless you
amend it, always starts with "Merge ...", and I think it is enough clue of
what the particular commit did.  Besides, I think letter "M" in the
pictorial part of the graph simply looks rather ugly.

I would even suggest not to use 'M' at all, whether its parents are
interesting or not.

Re: [PATCH 0/1] graph API: always print 'M' for merge commits

[Teemu Likonen]

Junio C Hamano wrote (2008-05-24 18:28 -0700):

> Adam Simpkins <adam@adamsimpkins.net> writes:
> 
> > Okay.  Here's a simple patch to restore the behavior of always
> > printing 'M' for merge commits, even if less than 2 parents are
> > shown in the graph.
> 
> I have to wonder if that is really necessary.  A merge commit, unless
> you amend it, always starts with "Merge ...", and I think it is enough
> clue of what the particular commit did.  Besides, I think letter "M"
> in the pictorial part of the graph simply looks rather ugly.
> 
> I would even suggest not to use 'M' at all, whether its parents are
> interesting or not.

I don't have strong opinions on this, although I prefer different
symbols for these conceptually different commits - in the current graph
view. But if in the future someone implements an option like
log.unicodegraph=true which displays the graph with nice Unicode chars,
then I think M's wouldn't quite fit there visually anymore.

Re: [PATCH 0/1] graph API: always print 'M' for merge commits

[Adam Simpkins]

On Sat, May 24, 2008 at 06:28:08PM -0700, Junio C Hamano wrote:
> Adam Simpkins <adam@adamsimpkins.net> writes:
> 
> > On Sat, May 24, 2008 at 09:25:30PM +0300, Teemu Likonen wrote:
> >> 
> >> I'd rather show merge commits always with M. They are still conceptually
> >> merges even if the current log view shows only one parent. This patch
> >> changes symbols * and M to be visual symbols, not logical symbols which
> >> I'd prefer.
> >> 
> >> For example user may be interested in finding certain merge but chooses
> >> to limit the output with a commit range. It helps if user can always
> >> trust the symbols * and M to have this conceptual difference.
> >
> > Okay.  Here's a simple patch to restore the behavior of always printing
> > 'M' for merge commits, even if less than 2 parents are shown in the
> > graph.
> 
> I have to wonder if that is really necessary.  A merge commit, unless you
> amend it, always starts with "Merge ...", and I think it is enough clue of
> what the particular commit did.  Besides, I think letter "M" in the
> pictorial part of the graph simply looks rather ugly.
> 
> I would even suggest not to use 'M' at all, whether its parents are
> interesting or not.

Either way is fine with me; it's just a cosmetic issue.
(We could even make it configurable, but that definitely seems like
overkill.)

-- 
Adam Simpkins
adam@adamsimpkins.net

Re: [PATCH 2/2] graph API: don't print branch lines for uninteresting merge parents

[Adam Simpkins]

On Fri, May 23, 2008 at 07:24:11PM -0700, Adam Simpkins wrote:
> Previously, the graphing code printed lines coming out of a merge commit
> for all of its parents, even if some of them were uninteresting.  Now it
> only prints lines for interesting commits.
> 
> For example, for a merge commit where only the first parent is
> interesting, the code now prints:
> 
>   *  merge commit
>   *  interesting child
> 
> instead of:
> 
>   M  merge commit
>   |\
>   *  interesting child

Whoops.  I forgot:

Signed-off-by: Adam Simpkins <adam@adamsimpkins.net>

-- 
Adam Simpkins
adam@adamsimpkins.net