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

From: Alex Riesen <raa.lkml@gmail.com>
Date: Nov 28, 2007, 21:10 UTC
Message-ID: <20071128211059.GA3173@steel.home>
In-Reply-To: <Pine.LNX.4.64.0711281211130.27959@racer.site>

Johannes Schindelin, Wed, Nov 28, 2007 13:18:10 +0100:

Show 8 quoted lines

> 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.

OTOH, I missed a diff-index interface where I could pass a resolved sha1 (the u8 array returned by get_sha1) and index state. Something like "int diff_tree_index(const unsigned char *sha1, struct rev_info *)".

Tempting, but I have only one use case for it. I don't actually know Git's code that well...

Show 9 quoted lines

> > 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)

Show 16 quoted lines

> > +	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". If I return "commitable" for a broken repo, it should fail elsewhere. If I return "not commitable" git-commit shall finish telling user there is nothing to commit, which is just wrong.

Show 6 quoted lines

> > +
> > +		init_revisions(&rev, "");
> > +		rev.abbrev = 0;
> > +		(void)setup_revisions(2, argv, &rev, NULL);
> 
> (void)?

Yep. Sorry, I just got carried away by recent tendency (here at work) to shut up PC-Lint (but please don't ask).

> 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.

> > +		(void)run_diff_index(&rev, 1 /* cached */);
> 
> (void)?

I'll remove them and resubmit. Stupid custom.

> 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).