Re: [PATCH] git-blame: Make the output human readable

From: Fredrik Kuivinen <freku045@student.liu.se>
Date: Mar 5, 2006, 12:38 UTC
Message-ID: <20060305123800.GD23448@c165.ib.student.liu.se>
In-Reply-To: <7vbqwlgkhk.fsf@assigned-by-dhcp.cox.net>

On Sun, Mar 05, 2006 at 04:10:47AM -0800, Junio C Hamano wrote:

Show 9 quoted lines

> Fredrik Kuivinen <freku045@student.liu.se> writes:
> 
> > The default output mode is slightly different from git-annotate's.
> > However, git-annotate's output mode can be obtained by using the
> > '-c' flag.
> 
> It might be better to default to human readable and make the
> script consumption format an option, if only to reduce typing.
>

The default output format is human readable, but it is different from the output format used by git-annotate. By default, (when the patch is applied) git-blame outputs lines with on the following form:

   <eight first digits of commit SHA1> (<first 15 letters of committer's name> YYYY-MM-DD HH:MM:SS TZ <line number, right justified>) file contents

where as git-annotate uses

   <eight first digits of commit SHA1>\t(<committer's name, right justified, padded to 10 characters> YYYY-MM-DD HH:MM:SS TZ\t<line number>)file contents

I find the first format easier to read since everything is aligned (we always output 15 characters for the committer's name padded with spaces if necessary and the line numbers are padded appropriately). It also takes up less space on screen since it doesn't use tabs.

However, I wanted to use the tests for git-annotate to test git-blame too and the tests do, of course, expect the output to be in git-annotate's format. Hence, the '-c' switch.

We may want to add an output format suitable for scripts too, which just lists the SHA1. But I don't think it is much more difficult to parse the format above, at least not if you just want the SHA1s.

Show 15 quoted lines

> > diff --git a/Makefile b/Makefile
> > index b6d8804..eb1887d 100644
> > --- a/Makefile
> > +++ b/Makefile
> > @@ -534,6 +534,10 @@ git-rev-list$X: rev-list.o $(LIB_FILE)
> >  	$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) $(filter %.o,$^) \
> >  		$(LIBS) $(OPENSSL_LIBSSL)
> >  
> > +git-blame$X: blame.o $(LIB_FILE)
> > +	$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) $(filter %.o,$^) \
> > +		$(LIBS) -lm
> > +
> 
> I wonder what it is about to link this binary different from others...
>

It uses log(3) to compute the number of digits needed to represent the last line number. It is probably better to this some other way though...

Show 21 quoted lines

> > +char* format_time(unsigned long time, const char* tz)
> > +{
> > +	static char time_buf[128];
> > +	time_t t = time;
> > +
> > +	strftime(time_buf, sizeof(time_buf), "%Y-%m-%d %H:%M:%S ", gmtime(&t));
> > +	strcat(time_buf, tz);
> > +	return time_buf;
> > +}
> 
> I think this shows GMT with time offset, which is compatible
> with the human readable time Johannes did to git-annotate.  I do
> not know what timezone CVS annotate shows its dates offhand (it
> seems to only show dates).  Johannes, is this an attempt to
> match what CVS does?
> 
> I am wondering if we want to be in line with the date formatting
> convention used for our commits and tags, that is, to show local
> timestamp with timezone.  The code to use would be show_date()
> from date.c if we go that route.
>

I think it is a good idea. Consistency is good.

- Fredrik