Bug in "git log --graph -p -m" (version 1.7.7.6)

Bug in "git log --graph -p -m" (version 1.7.7.6)

[Dale R. Worley]

I have found a situation where "git log" produces (apparently)
endless output.  Presumably this is a bug.  Following is a (Linux)
script that reliably reproduces the error for me (on Fedora 16):

----------
set -ve

# Print the git version.
git --version

# Create respository.
rm -rf .git
git init

# Initial commit.
( echo 1 ; echo 2 ; echo 3 ) >file
git add file
git commit -m 'Commit P'
git branch B HEAD

# Next commit on master adds line "1a".
( echo 1 ; echo 1a ; echo 2 ; echo 3 ) >file
git add file
git commit -m 'Commit Q'

git checkout B

# Next commit on B adds line "2a".
( echo 1 ; echo 2 ; echo 2a ; echo 3 ) >file
git add file
git commit -m 'Commit R'

# Merge the two commits, but add line "3a" to the commit as well.
git checkout master
git merge --no-commit B
# Show what the merge produces.
cat file
# Add line "3a".
( echo 1 ; echo 1a ; echo 2 ; echo 2a ; echo 3 ; echo 3a ) >file
git commit -m 'Commit S'

# These log commands work.
git log
git log --graph
git log --graph -p

# This log command produces infinite output.
git log --graph -p -m
----------

Dale

Re: Bug in "git log --graph -p -m" (version 1.7.7.6)

[Junio C Hamano]

worley@alum.mit.edu (Dale R. Worley) writes:

> I have found a situation where "git log" produces (apparently)
> endless output.  Presumably this is a bug.  Following is a (Linux)
> script that reliably reproduces the error for me (on Fedora 16):

Wasn't this fixed at v1.8.1.1~13 or is this a different issue?

Re: Bug in "git log --graph -p -m" (version 1.7.7.6)

[Matthieu Moy]

Junio C Hamano <gitster@pobox.com> writes:

> worley@alum.mit.edu (Dale R. Worley) writes:
>
>> I have found a situation where "git log" produces (apparently)
>> endless output.  Presumably this is a bug.  Following is a (Linux)
>> script that reliably reproduces the error for me (on Fedora 16):
>
> Wasn't this fixed at v1.8.1.1~13 or is this a different issue?

In any case, I can't reproduce with 1.8.1.2.526.gf51a757: I don't get
undless output. On the other hand, I get a slightly misformatted output:

*   commit a393ed598e9fb11436f85bd58f1a38c82f2cadb7 (from 2c1e6a36f4b712e914fac994463da7d0fdb2bc6d)
|\  Merge: 2c1e6a3 33e70e7
| | Author: Matthieu Moy <Matthieu.Moy@imag.fr>
| | Date:   Tue Feb 5 22:05:33 2013 +0100
| | 
| |     Commit S
| | 
| | diff --git a/file b/file
| | index 6bb4d3e..afd2e75 100644
| | --- a/file
| | +++ b/file
| | @@ -1,4 +1,5 @@
| |  1
| |  1a
| |  2
| | +2a
| |  3
| | 
commit a393ed598e9fb11436f85bd58f1a38c82f2cadb7 (from 33e70e70c0173d634826b998bdc304f93c0966b8)
| | Merge: 2c1e6a3 33e70e7
| | Author: Matthieu Moy <Matthieu.Moy@imag.fr>
| | Date:   Tue Feb 5 22:05:33 2013 +0100

The second "commit" line (diff with second parent) doesn't have the
"| |" prefix, I don't think this is intentional.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

Re: Bug in "git log --graph -p -m" (version 1.7.7.6)

[Dale R. Worley]

> From: Matthieu Moy <Matthieu.Moy@grenoble-inp.fr>
> 
> In any case, I can't reproduce with 1.8.1.2.526.gf51a757: I don't get
> undless output. On the other hand, I get a slightly misformatted output:
> 
> *   commit a393ed598e9fb11436f85bd58f1a38c82f2cadb7 (from 2c1e6a36f4b712e914fac994463da7d0fdb2bc6d)
> |\  Merge: 2c1e6a3 33e70e7
> | | Author: Matthieu Moy <Matthieu.Moy@imag.fr>
> | | Date:   Tue Feb 5 22:05:33 2013 +0100
> | | 
> | |     Commit S
> | | 
> | | diff --git a/file b/file
> | | index 6bb4d3e..afd2e75 100644
> | | --- a/file
> | | +++ b/file
> | | @@ -1,4 +1,5 @@
> | |  1
> | |  1a
> | |  2
> | | +2a
> | |  3
> | | 
> commit a393ed598e9fb11436f85bd58f1a38c82f2cadb7 (from 33e70e70c0173d634826b998bdc304f93c0966b8)
> | | Merge: 2c1e6a3 33e70e7
> | | Author: Matthieu Moy <Matthieu.Moy@imag.fr>
> | | Date:   Tue Feb 5 22:05:33 2013 +0100
> 
> The second "commit" line (diff with second parent) doesn't have the
> "| |" prefix, I don't think this is intentional.

The second "commit" line should start with "| * ":

> | | 
> | * commit a393ed598e9fb11436f85bd58f1a38c82f2cadb7 (from 33e70e70c0173d634826b998bdc304f93c0966b8)
> | | Merge: 2c1e6a3 33e70e7
> | | Author: Matthieu Moy <Matthieu.Moy@imag.fr>
> | | Date:   Tue Feb 5 22:05:33 2013 +0100

Dale

Re: Bug in "git log --graph -p -m" (version 1.7.7.6)

[John Keeping]

On Wed, Feb 06, 2013 at 10:03:00AM -0500, Dale R. Worley wrote:
> > From: Matthieu Moy <Matthieu.Moy@grenoble-inp.fr>
> > 
> > In any case, I can't reproduce with 1.8.1.2.526.gf51a757: I don't get
> > undless output. On the other hand, I get a slightly misformatted output:
> > 
> > *   commit a393ed598e9fb11436f85bd58f1a38c82f2cadb7 (from 2c1e6a36f4b712e914fac994463da7d0fdb2bc6d)
> > |\  Merge: 2c1e6a3 33e70e7
> > | | Author: Matthieu Moy <Matthieu.Moy@imag.fr>
> > | | Date:   Tue Feb 5 22:05:33 2013 +0100
> > | | 
> > | |     Commit S
> > | | 
> > | | diff --git a/file b/file
> > | | index 6bb4d3e..afd2e75 100644
> > | | --- a/file
> > | | +++ b/file
> > | | @@ -1,4 +1,5 @@
> > | |  1
> > | |  1a
> > | |  2
> > | | +2a
> > | |  3
> > | | 
> > commit a393ed598e9fb11436f85bd58f1a38c82f2cadb7 (from 33e70e70c0173d634826b998bdc304f93c0966b8)
> > | | Merge: 2c1e6a3 33e70e7
> > | | Author: Matthieu Moy <Matthieu.Moy@imag.fr>
> > | | Date:   Tue Feb 5 22:05:33 2013 +0100
> > 
> > The second "commit" line (diff with second parent) doesn't have the
> > "| |" prefix, I don't think this is intentional.
> 
> The second "commit" line should start with "| * ":

No.  That would indicate a commit on the branch that is the second
parent of the first commit.  But this is the same commit as the one
above, just with a diff against its second parent instead of its first
parent.

I would argue that the line should start with "| | ", since it really is
just a continuation of the same commit.

| | 
| | commit a393ed598e9fb11436f85bd58f1a38c82f2cadb7 (from 33e70e70c0173d634826b998bdc304f93c0966b8)
| | Merge: 2c1e6a3 33e70e7
| | Author: Matthieu Moy <Matthieu.Moy@imag.fr>
| | Date:   Tue Feb 5 22:05:33 2013 +0100


John

Re: Bug in "git log --graph -p -m" (version 1.7.7.6)

[Matthieu Moy]

John Keeping <john@keeping.me.uk> writes:

> I would argue that the line should start with "| | ", since it really is
> just a continuation of the same commit.
>
> | | 
> | | commit a393ed598e9fb11436f85bd58f1a38c82f2cadb7 (from 33e70e70c0173d634826b998bdc304f93c0966b8)
> | | Merge: 2c1e6a3 33e70e7
> | | Author: Matthieu Moy <Matthieu.Moy@imag.fr>
> | | Date:   Tue Feb 5 22:05:33 2013 +0100

Yes.

I had a look at the code, I guess the call to graph_show_commit() in
show_log() (in log-tree.c) should have called graph_show_padding() but
didn't in this case. Then I got lost in graph.c :-(.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

[PATCH] graph: output padding for merge subsequent parents

[John Keeping]

On Wed, Feb 06, 2013 at 07:33:08PM +0100, Matthieu Moy wrote:
> John Keeping <john@keeping.me.uk> writes:
> 
> > I would argue that the line should start with "| | ", since it really is
> > just a continuation of the same commit.
> >
> > | | 
> > | | commit a393ed598e9fb11436f85bd58f1a38c82f2cadb7 (from 33e70e70c0173d634826b998bdc304f93c0966b8)
> > | | Merge: 2c1e6a3 33e70e7
> > | | Author: Matthieu Moy <Matthieu.Moy@imag.fr>
> > | | Date:   Tue Feb 5 22:05:33 2013 +0100
> 
> Yes.
> 
> I had a look at the code, I guess the call to graph_show_commit() in
> show_log() (in log-tree.c) should have called graph_show_padding() but
> didn't in this case. Then I got lost in graph.c :-(.

I think this is the correct answer.  But now I've found that "git log
--graph -c -p" doesn't indent the diff - that seems to be a separate
issue.

-- >8 --

When showing merges in git-log, the same commit is shown once for each
parent.  Combined with "--graph" this results in graph_show_commit()
being called once for each parent without graph_update() being called.

Currently graph_show_commit() does not print anything on subsequent
invocations for the same commit (this was changed by commit 656197a -
"graph.c: infinite loop in git whatchanged --graph -m" from the previous
behaviour of looping infinitely).

Change this so that if the graph code believes it has already shown the
commit it prints a single padding line.

Signed-off-by: John Keeping <john@keeping.me.uk>
---
 graph.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/graph.c b/graph.c
index 391a712..2a3fc5c 100644
--- a/graph.c
+++ b/graph.c
@@ -1227,6 +1227,16 @@ void graph_show_commit(struct git_graph *graph)
 	if (!graph)
 		return;
 
+	/*
+	 * When showing a diff of a merge against each of its parents, we
+	 * are called once for each parent without graph_update having been
+	 * called.  In this case, simply output a single padding line.
+	 */
+	if (graph_is_commit_finished(graph)) {
+		graph_show_padding(graph);
+		shown_commit_line = 1;
+	}
+
 	while (!shown_commit_line && !graph_is_commit_finished(graph)) {
 		shown_commit_line = graph_next_line(graph, &msgbuf);
 		fwrite(msgbuf.buf, sizeof(char), msgbuf.len, stdout);
-- 
1.8.1.2

Re: [PATCH] graph: output padding for merge subsequent parents

[Matthieu Moy]

John Keeping <john@keeping.me.uk> writes:

> diff --git a/graph.c b/graph.c
> index 391a712..2a3fc5c 100644
> --- a/graph.c
> +++ b/graph.c
> @@ -1227,6 +1227,16 @@ void graph_show_commit(struct git_graph *graph)
>  	if (!graph)
>  		return;
>  
> +	/*
> +	 * When showing a diff of a merge against each of its parents, we
> +	 * are called once for each parent without graph_update having been
> +	 * called.  In this case, simply output a single padding line.
> +	 */
> +	if (graph_is_commit_finished(graph)) {
> +		graph_show_padding(graph);
> +		shown_commit_line = 1;
> +	}
> +
>  	while (!shown_commit_line && !graph_is_commit_finished(graph)) {

This works, but if we know we're not going to enter the while loop, it
seams even easier to do this:

--- a/graph.c
+++ b/graph.c
@@ -1227,7 +1227,17 @@ void graph_show_commit(struct git_graph *graph)
        if (!graph)
                return;
 
-       while (!shown_commit_line && !graph_is_commit_finished(graph)) {
+       /*
+        * When showing a diff of a merge against each of its parents, we
+        * are called once for each parent without graph_update having been
+        * called.  In this case, simply output a single padding line.
+        */
+       if (graph_is_commit_finished(graph)) {
+               graph_show_padding(graph);
+               return;
+       }
+
+       while (!shown_commit_line) {
                shown_commit_line = graph_next_line(graph, &msgbuf);
                fwrite(msgbuf.buf, sizeof(char), msgbuf.len, stdout);
                if (!shown_commit_line)


-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

Re: [PATCH] graph: output padding for merge subsequent parents

[Junio C Hamano]

Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:

> John Keeping <john@keeping.me.uk> writes:
>
>> diff --git a/graph.c b/graph.c
>> index 391a712..2a3fc5c 100644
>> --- a/graph.c
>> +++ b/graph.c
>> @@ -1227,6 +1227,16 @@ void graph_show_commit(struct git_graph *graph)
>>  	if (!graph)
>>  		return;
>>  
>> +	/*
>> +	 * When showing a diff of a merge against each of its parents, we
>> +	 * are called once for each parent without graph_update having been
>> +	 * called.  In this case, simply output a single padding line.
>> +	 */
>> +	if (graph_is_commit_finished(graph)) {
>> +		graph_show_padding(graph);
>> +		shown_commit_line = 1;
>> +	}
>> +
>>  	while (!shown_commit_line && !graph_is_commit_finished(graph)) {
>
> This works, but if we know we're not going to enter the while loop, it
> seams even easier to do this:
>
> --- a/graph.c
> +++ b/graph.c
> @@ -1227,7 +1227,17 @@ void graph_show_commit(struct git_graph *graph)
>         if (!graph)
>                 return;
>  
> -       while (!shown_commit_line && !graph_is_commit_finished(graph)) {
> +       /*
> +        * When showing a diff of a merge against each of its parents, we
> +        * are called once for each parent without graph_update having been
> +        * called.  In this case, simply output a single padding line.
> +        */
> +       if (graph_is_commit_finished(graph)) {
> +               graph_show_padding(graph);
> +               return;
> +       }
> +
> +       while (!shown_commit_line) {
>                 shown_commit_line = graph_next_line(graph, &msgbuf);
>                 fwrite(msgbuf.buf, sizeof(char), msgbuf.len, stdout);
>                 if (!shown_commit_line)

In this particular case, with the current state of this function, it
is probably OK, but an early return like this tend to be a source of
future bugs in the longer term, to make the codeflow skip whatever
necessary clean-up that needs to be done after the loop exits.