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

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Nov 28, 2007, 12:18 UTC
Message-ID: <Pine.LNX.4.64.0711281211130.27959@racer.site>
In-Reply-To: <20071127214425.GA3156@steel.home>

Hi,

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

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

Show 13 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?

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

(void)?

Besides, would this not be more elegant as

		setup_revisions(0, NULL, &rev, "HEAD");

Hmm?

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

(void)?

Other than that (including my remark about refactoring that piece of code), I like it.

Thanks, Dscho