Re: [PATCH] Make gc a builtin.

["Shawn O. Pearce" <spearce@spearce.org>]

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

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

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