[PATCH 0/6] Bunch of new features for cg-log and cg-diff

[PATCH 0/6] Bunch of new features for cg-log and cg-diff

[Dan Holmsand]

This series adds optget-style option parsing, support for almost all
git-diff-* features, git-apply --stat support, common colorization code, 
better performance for cg-log and some other stuff.

/dan

Re: [PATCH 0/6] Bunch of new features for cg-log and cg-diff

[Jonas Fonseca]

Dan Holmsand <holmsand@gmail.com> wrote Thu, Jun 09, 2005:
> This series adds optget-style option parsing, support for almost all
> git-diff-* features, git-apply --stat support, common colorization code, 
> better performance for cg-log and some other stuff.

I tried out your patchset and have a few comments ...

cg-diff:

 - The pager is only used when passing -c. Is that intentional?

 - Nice with the diffstat option.

cg-log:

 - In the non-verbose summary you use the author date. One motivation
   for using the commit date is that the summary output makes it easy to
   track 'activity' and see if/when your patch made it in. Maybe I've
   just become too used to CVS changelogs.

 - Even though the more dense time format in the summary output is a
   nice idea the new date information is unfortunately also makes the
   summary output less useful, IMO. It can even make the by-date
   scanning harder because you have to jump between two significantly
   different date formats. With the new verbose distinction there should
   be no need for making the date so dense.

I don't much like the inverted colors caused by the searching. Although
the quick goto next entry thing is nice the colors can be very
intrusive, and having to search for some nonsense string to remove them
is terrible.

What about a COGITO_COLORS environment variable for configuring what
string setup_colors() will work on. It could maybe take the place of the
COGITO_AUTO_COLOR environment variable although this is two different
things.

With the long help output of cg-log maybe we should consider also
displaying it in a pager.

A minor note about the option parsing. cg-log -sh will give the error

	cg-log: unrecoginized option `-h'

-- 
Jonas Fonseca

Re: [PATCH 0/6] Bunch of new features for cg-log and cg-diff

[Dan Holmsand]

Jonas Fonseca wrote:
> I tried out your patchset and have a few comments ...
> 
> cg-diff:
> 
>  - The pager is only used when passing -c. Is that intentional?

Yes. The reasoning was that people wanting more features probably will 
use color as well (and I didn't want to force the pager on people who 
use cg-diff just to check *if* something changed).

But I'm not 100% convinced I'm doing the right thing. Any suggestions?

> cg-log:
> 
>  - In the non-verbose summary you use the author date. One motivation
>    for using the commit date is that the summary output makes it easy to
>    track 'activity' and see if/when your patch made it in. Maybe I've
>    just become too used to CVS changelogs.

Yeah, maybe :-) Seriously, I think author date carries more information, 
particularly since the log is already (most of the time) ordered by 
commit date. So, when you see an old date before a newer one, you 
immediately know that some old stuff was incorporated into the repository.

Also, git-rev-list --pretty uses author date, and I wanted to be consistent.

Of course, I could always add one more option :-)

>  - Even though the more dense time format in the summary output is a
>    nice idea the new date information is unfortunately also makes the
>    summary output less useful, IMO. It can even make the by-date
>    scanning harder because you have to jump between two significantly
>    different date formats. With the new verbose distinction there should
>    be no need for making the date so dense.

This is also a matter of taste, obviously. I actually *like* having two 
different formats, as it makes the difference between "today" and 
"earlier" more obvious. And I don't really care what time of day 
something was written three weeks ago.

Perhaps this should be customisable as well, if it's felt to be 
important enough?

> I don't much like the inverted colors caused by the searching. Although
> the quick goto next entry thing is nice the colors can be very
> intrusive, and having to search for some nonsense string to remove them
> is terrible.

"export LESS=-G" will do what you want. Maybe that should be the default?

> What about a COGITO_COLORS environment variable for configuring what
> string setup_colors() will work on. It could maybe take the place of the
> COGITO_AUTO_COLOR environment variable although this is two different
> things.

That should already be there. "COGITO_COLORS='header=31' cg-log" should 
give very red headers, for example.

> With the long help output of cg-log maybe we should consider also
> displaying it in a pager.

Good idea.

> A minor note about the option parsing. cg-log -sh will give the error
> 
> 	cg-log: unrecoginized option `-h'
> 

Yeah, I know. "-h" and "--help" are the only options not handled by 
optparse in cg-log. I thought I could rely on the general help-finding 
logic in cg-Xlib for that.

On the other hand, you really shouldn't say "cg-log -sh" :-)

/dan

Re: [PATCH 0/6] Bunch of new features for cg-log and cg-diff

[Dan Holmsand]

Jonas Fonseca wrote:
> I tried out your patchset and have a few comments ...
> 
> cg-diff:
> 
>  - The pager is only used when passing -c. Is that intentional?

Yes. The reasoning was that people wanting more features probably will 
use color as well (and I didn't want to force the pager on people who 
use cg-diff just to check *if* something changed).

But I'm not 100% convinced I'm doing the right thing. Any suggestions?

> cg-log:
> 
>  - In the non-verbose summary you use the author date. One motivation
>    for using the commit date is that the summary output makes it easy to
>    track 'activity' and see if/when your patch made it in. Maybe I've
>    just become too used to CVS changelogs.

Yeah, maybe :-) Seriously, I think author date carries more information, 
particularly since the log is already (most of the time) ordered by 
commit date. So, when you see an old date before a newer one, you 
immediately know that some old stuff was incorporated into the repository.

Also, git-rev-list --pretty uses author date, and I wanted to be consistent.

Of course, I could always add one more option :-)

>  - Even though the more dense time format in the summary output is a
>    nice idea the new date information is unfortunately also makes the
>    summary output less useful, IMO. It can even make the by-date
>    scanning harder because you have to jump between two significantly
>    different date formats. With the new verbose distinction there should
>    be no need for making the date so dense.

This is also a matter of taste, obviously. I actually *like* having two 
different formats, as it makes the difference between "today" and 
"earlier" more obvious. And I don't really care what time of day 
something was written three weeks ago.

Perhaps this should be customisable as well, if it's felt to be 
important enough?

> I don't much like the inverted colors caused by the searching. Although
> the quick goto next entry thing is nice the colors can be very
> intrusive, and having to search for some nonsense string to remove them
> is terrible.

"export LESS=-G" will do what you want. Maybe that should be the default?

> What about a COGITO_COLORS environment variable for configuring what
> string setup_colors() will work on. It could maybe take the place of the
> COGITO_AUTO_COLOR environment variable although this is two different
> things.

That should already be there. "COGITO_COLORS='header=31' cg-log" should 
give very red headers, for example.

> With the long help output of cg-log maybe we should consider also
> displaying it in a pager.

Good idea.

> A minor note about the option parsing. cg-log -sh will give the error
> 
> 	cg-log: unrecoginized option `-h'
> 

Yeah, I know. "-h" and "--help" are the only options not handled by 
optparse in cg-log. I thought I could rely on the general help-finding 
logic in cg-Xlib for that.

On the other hand, you really shouldn't say "cg-log -sh" :-)

/dan