Re: [PATCH] Make gc a builtin.

From: Shawn O. Pearce <spearce@spearce.org>
Date: Mar 12, 2007, 14:43 UTC
Message-ID: <20070312144312.GE15150@spearce.org>
In-Reply-To: <3f80363f0703111951x9d88e74x8d7723af97c18c7@mail.gmail.com>

A good (second) try.

James Bowes <jbowes@dangerouslyinc.com> wrote:

> diff --git a/builtin-gc.c b/builtin-gc.c
> +
> +static int pack_refs;

Actually I think you want to use:

static int pack_refs = -1;

See below for why...

Show 11 quoted lines

> +static int gc_config(const char *var, const char *value)
> +{
> +	if (!strcmp(var, "gc.packrefs"))
> +		if (strlen(value) == 0 || !strcmp(value, "notbare"))
> +			pack_refs = !is_bare_repository();
> +		else
> +			pack_refs = git_config_bool(var, value);
> +	else
> +		return git_default_config(var, value);
> +	return 0;
> +}

Gaaah. How about some curly braces around the then part of that first if?

Actually, we typically just write this more like:

static int gc_config(const char *var, const char *value)
{
	if (!strcmp(var, "gc.packrefs")) {
		if (!strcmp(value, "notbare"))
			pack_refs = -1;
		else
			pack_refs = git_config_bool(var, value);
	}
	return git_default_config(var, value);
}

Show 6 quoted lines

> +int cmd_gc(int argc, const char **argv, const char *prefix)
> +{
> +	int i;
> +	int prune = 0;
> +
> +	git_config(gc_config);

if (pack_refs < 0)
	pack_refs = !is_bare_repository();

The is_bare_repository function guesses until the configuration is done parsing; once the configuration has been parsed it has a definate answer one way or the other. So what I'm suggesting you do here is set pack_refs = -1 to mean use the is_bare_repository setting, otherwise it stays what it was set to.

> +    if (pack_refs)
> +	    if (run_command_v_opt(argv_pack_refs, RUN_GIT_CMD))
> +            goto failure;

....

> +    if (prune)
> +        if (run_command_v_opt(argv_prune, RUN_GIT_CMD))
> +            goto failure;

Gaah. Tabs-vs-spaces, not to mention that these aren't even lining up the same way. I too prefer what Dsco suggested already:

	if (prune && run_command_v_opt(argv_prune, RUN_GIT_CMD))
		return error("failed to run %s", argv_prune[0]);

-- 
Shawn.