From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Mon, 12 Mar 2007 03:07:43 GMT
Subject: Re: [PATCH 2/2] Make gc a builtin.
Message-ID: <Pine.LNX.4.63.0703120403360.22628@wbgn013.biozentrum.uni-wuerzburg.de>
In-Reply-To: <7vtzwrtdmx.fsf@assigned-by-dhcp.cox.net>

Hi,

On Sun, 11 Mar 2007, Junio C Hamano wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> > And instead of die()ing, I'd rather do something like
> >
> > 	return (pack_refs || run_command_v_opt(argv_pack_refs, RUN_GIT_CMD) &&
> > 		run_command_v_opt(argv_reflog_expire, RUN_GIT_CMD) &&
> > 		run_command_v_opt(argv_repack, RUN_GIT_CMD) &&
> > 		(prune || run_command_v_opt(argv_prune, RUN_GIT_CMD) &&
> > 		run_command_v_opt(argv_rerere, RUN_GIT_CMD);
> 
> Gaaaaaaaah.
> 
> That may be valid C,

Actually, it is not. As usual, I fscked up: run_command_v_opt() is 
supposed to return 0 on _success_, so all the "&&" should be "||", and all 
the "||" should be "&&".

> 	if (we are told to pack-refs)
>         	if (try to pack refs and find error)
> 			goto failure;

I find this not very elegant. Instead, I'd do

	if (do_pack_refs && run_comand_v_opt(argv_pack_refs, RUN_GIT_CMD))
		return error("Could not run pack-refs.");

It does not only avoid the evil goto, but uses the screen estate for some 
nice error messages so that the user is not left out in the cold when 
something is wrong.

Ciao,
Dscho