Re: [PATCH/RFC/GSOC] make git-pull a builtin

[Johannes Schindelin <johannes.schindelin@gmx.de>]

Hi Paul,

thank you for this very detailed mail. It was a real pleasure to read this well-researched document.

In the following, I will pick out only parts from the mail, in the interest of both of our time. Please assume that I agree with everything that I do not quote below (and even the with quoted parts I agree mostly ;-)).

On 2015-03-17 14:57, Paul Tan wrote:

> on Windows the runtime fell from 8m 25s to 1m 3s.

This is *exactly* the type of benefit that makes this project so important! Nice one.

> A simpler, but less perfect strategy might be to just convert the shell
> scripts directly statement by statement to C, using the run_command*()
> functions as Duy Nguyen[2] suggested, before changing the code to use
> the internal API.

Yeah, the idea is to have a straight-forward strategy to convert the scripts in as efficient manner as possible. It also makes reviewing easier if the first step is an almost one-to-one translation to `run_command*()`-based builtins.

Plus, it is rewarding to have concise steps that can be completed in a timely manner.


> +/* NOTE: git-pull.sh only supports --log and --no-log, as opposed to what
> + * man git-pull says. */
> +static int opt_shortlog_len;

This comment might want to live in the commit message instead... It is prone to get stale in the code while it will always be correct (and easier to spot) in the commit message.

> +/**
> + * Returns default rebase option value
> + */
> +static int rebase_config_default(void)
> +{
> +	struct strbuf name = STRBUF_INIT;
> +	const char *value = NULL;
> +	int boolval;
> +
> +	strbuf_addf(&name, "branch.%s.rebase", head_name_short);
> +	if (git_config_get_value(name.buf, &value))
> +		git_config_get_value("pull.rebase", &value);
> +	strbuf_release(&name);
> +	if (!value)
> +		return REBASE_FALSE;
> +	boolval = git_config_maybe_bool("pull.rebase", value);
> +	if (boolval >= 0)
> +		return boolval ? REBASE_TRUE : REBASE_FALSE;
> +	else if (value && !strcmp(value, "preserve"))
> +		return REBASE_PRESERVE;
> +	die(_("invalid value for branch.%s.rebase \"%s\""), head_name_short, value);
> +}

Personally, I would use a couple of empty lines for enhanced structure. Conceptually, there are four parts: the variable declarations, querying the config, parsing the value, and `die()`ing in case of error. There is already an empty line between the first two parts, and I would imagine that readability would be improved further by having an empty line after the `strbuf_release()` and the `return REBASE_PRESERVE` statement, respectively.

> +static int parse_opt_rebase(const struct option *opt, const char
> *arg, int unset)
> +{
> +	if (arg)
> +		*(int *)opt->value = parse_rebase(arg);
> +	else
> +		*(int *)opt->value = unset ? REBASE_FALSE : REBASE_TRUE;
> +	return (*(int *)opt->value) >= 0 ? 0 : -1;
> +}

In this function (and also in other places below), there is this pattern that a `struct option` pointer is passed to the function, but then only `*(int *)opt->value` is written to. Therefore, I would suggest to change the signature of the function and pass `(int *)opt->value` as function parameter.

> +static int has_unstaged_changes(void)

Yeah, this function, as well as the ones below it, look as if they are so common that they *should* be already somewhere in libgit.a. But I did not find them, either...

Of course it *would* be nice to identify places where essentially the same code is needed, and refactor accordingly. But I think that is outside the scope of this project.

The rest looks pretty good (and you realize that my comments above are really more nit picks than anything else).

The FIXMEs should be relatively easy to address. It would be my pleasure to work with you.

Ciao,
Johannes