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

From: Adam Simpkins <adam@adamsimpkins.net>
Date: May 24, 2008, 01:05 UTC
Message-ID: <20080524010555.GA5385@adamsimpkins.net>
In-Reply-To: <7vve14rbu7.fsf@gitster.siamese.dyndns.org>

On Fri, May 23, 2008 at 01:42:56PM -0700, Junio C Hamano wrote:

Show 35 quoted lines

> 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