Re: [PATCH 1/4] Add history graph API

From: Adam Simpkins <adam@adamsimpkins.net>
Date: Apr 7, 2008, 08:34 UTC
Message-ID: <20080407083412.GA9764@adamsimpkins.net>
In-Reply-To: <200804070824.10764.tlikonen@iki.fi>

On Mon, Apr 07, 2008 at 08:24:10AM +0300, Teemu Likonen wrote:

Show 23 quoted lines

> Adam Simpkins kirjoitti:
> 
> > Actually, going back and testing this, it looks like I have a bug
> > when handling --graph together with --pretty=format.  There's a
> > missing newline after the user's format message and the next graph
> > line.  I'll try to fix this and submit a patch later this evening.
> 
> Also, the output is not indented for options that display some 
> additional information to commit message. Those include:
> 
> --raw
> --stat
> --numstat
> --shortstat
> --summary
> --name-only
> --name-status
> 
> I'm not sure if the diff output of -p, -u etc. should be 
> indented--probably not--but for different stat and summary options it 
> would be nice to not have their output displayed over the graph area. 
> Especially --name-status is funny since it displays "M" to column 1 to 
> indicate modified file while "M" also means merge commit in the graph.

Hmm. This is a harder problem to fix. All of the options you list above are handled by the internal diff API. The diff API doesn't have any knowledge about log and rev-list options, such as --graph.

The nicest way to fix this would probably be to write new diff API functions that output to a strbuf instead of printing directly to stdout. Then the log code could prefix each line of the buffer with the graph info before printing it.

However, this would be a lot of work, and I'm not sure that it's really worth the effort at the moment. For now, I'm leaning towards changing the code to just exit with an error if --graph is used with any of these options.

Any opinions?  Alternative suggestions?

-- 
Adam Simpkins
adam@adamsimpkins.net