Re: [PATCH] Do not generate full commit log message if it not going to be used

[Johannes Schindelin <Johannes.Schindelin@gmx.de>]

Hi,

On Wed, 28 Nov 2007, Alex Riesen wrote:

> Johannes Schindelin, Wed, Nov 28, 2007 13:18:10 +0100:
> > On Tue, 27 Nov 2007, Alex Riesen wrote:
> > 
> > > Could not stop myself. Hopefully didn't beat anyone to it :)
> > > Almost all code shamelessly stolen from builtin-diff-index.c.
> > 
> > Then I have to wonder if it would not be a better idea to refactor the 
> > code, so that other people do not have to steal the code again, but are 
> > able to reuse it ;-)
> 
> Not sure it will be worth the effort. It is really short.

Okay.

> > > Preprocessor trickery in DIFF_OPT_* macros is disgusting, it breaks Vim 
> > > word completion and trying to use many flags in one expression looks 
> > > just ugly.
> > 
> > How does it break Vim word completion?  And why should something like
> > 
> > 		DIFF_OPT_SET(&rev.diffopt, QUIET | EXIT_WITH_STATUS);
> > 
> > look ugly?  I find it highly readable.
> 
> Oh, this would look ok. It just wont compile: DIFF_OPT_SET prepends
> second argument with DIFF_OPT_:
> 
> #define DIFF_OPT_TST(opts, flag)    ((opts)->flags & DIFF_OPT_##flag)
> #define DIFF_OPT_SET(opts, flag)    ((opts)->flags |= DIFF_OPT_##flag)
> #define DIFF_OPT_CLR(opts, flag)    ((opts)->flags &= ~DIFF_OPT_##flag)

Ouch.  Sorry for suggesting it when I clearly had no clue.

> > > +	if (no_edit) {
> > > +		static const char *argv[] = { NULL, "HEAD", NULL };
> > > +		struct rev_info rev;
> > > +		unsigned char sha1[40];
> > > +		int is_initial;
> > > +
> > > +		fclose(fp);
> > > +
> > > +		if (!active_nr && read_cache() < 0)
> > > +			die("Cannot read index");
> > > +
> > > +		if (get_sha1("HEAD", sha1) != 0)
> > > +			return !!active_nr;
> > 
> > Don't want to be anal here, but are there possibly reasons (read "possible 
> > errors") other than an empty repo where this triggers?
> 
> Definitely. I just don't know. OTOH, I can only return "committable" or 
> "not committable".

I guess it is good enough.  Just wanted to point out that this can fail if 
.git/HEAD is not readable.

> > Besides, would this not be more elegant as
> > 
> > 		setup_revisions(0, NULL, &rev, "HEAD");
> 
> Hmm... And I was so puzzled as to what that "def" argument could
> possibly mean... Still am, in fact. But it works.

It is the mechanism which makes "git log" default to "git log HEAD".

> > Other than that (including my remark about refactoring that piece of 
> > code), I like it.
> 
> Me too: I have *extensively* tested it today and a commit on the
> 2.6GHz/2Gb/SATA windows machine is almost as fast as on my linux
> laptop now (Centrino/1.2GHz downclocked to 800MHz/384Mb/IDE).

Yes, I suspected something like this would happen.

Ciao,
Dscho