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

From: Matthieu Moy <matthieu.moy@grenoble-inp.fr>
Date: Mar 18, 2015, 17:52 UTC
Message-ID: <vpqwq2eyyzl.fsf@anie.imag.fr>
In-Reply-To: <1426600662-32276-1-git-send-email-pyokagan@gmail.com>

Hi,

First of all, thanks a lot for working on this. I'm rather impressed to see a working proof of concept so soon! And impressed by the quality for a "first draft".

A few minor remaks below after a very quick look.

Paul Tan <pyokagan@gmail.com> writes:

> Ideally, I think the solution is to
> improve the test suite and make it as comprehensive as possible, but
> writing a comprehensive test suite may be too time consuming.

time-consuming, but also very beneficial since the code would end up being better tested. For sure, a rewrite is a good way to break stuff, but anything untested can also be broken by mistake rather easily at any time.

I'd suggest doing a bit of manual mutation testing: take your C code, comment-out a few lines of code, see if the tests still pass, and if they do, add a failing test that passes again once you uncomment the code.

Show 9 quoted lines

> diff --git a/Makefile b/Makefile
> index 44f1dd1..50a6a16 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -470,7 +470,6 @@ SCRIPT_SH += git-merge-octopus.sh
>  SCRIPT_SH += git-merge-one-file.sh
>  SCRIPT_SH += git-merge-resolve.sh
>  SCRIPT_SH += git-mergetool.sh
> -SCRIPT_SH += git-pull.sh

When converting a script into a builtin, we usually move the old script to contrib/examples/.

> +static const char * const builtin_pull_usage[] = {
> +	N_("git pull [-n | --no-stat] [--[no-]commit] [--[no-]squash] [--[no-]ff|--ff-only] [--[no-]rebase|--rebase=preserve] [-s strategy]... [<fetch-options>] <repo> <head>..."),

I know we have many instances of very long lines for usage string, but it would be better IMHO to wrap it both in the code and in the output of "git pull -h".

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

We usually write multi-line comments

/*
 * like
 * this
 */

> +/* Global vars since they are used often */

Being use often does not count as an excuse for being global IMHO. Having global variables means you share the same instance in several functions, and you have to be careful with things like

void g()
{
	glob = bar;
}

void f()
{
	glob = foo;
	g();
	bar = glob;
}

As a reader, I'd rather not have to be careful about this to keep my neurons free for other things.

> +static char *head_name;

Actually, this one is used only in one function, so "often" is not even true ;-).

> +static struct option builtin_pull_options[] = {

You may also declare this as local in cmd_pull().

> +/**
> + * Returns remote for branch

Here and elsewhere: use imperative (return, not return_s_). The comment asks the function to return a value.

> +	strbuf_addf(&key, "branch.%s.remote", branch);
> +	git_config_get_value(key.buf, &remote);
> +	strbuf_release(&key);

This config API is beautiful :-).

(Before last year's GSoC, you'd have needed ~10 lines of code to do the same thing)

> +		return error("Ambiguous refname: '%s'", ref);

Here and elsewhere: don't forget to mark strings for translation.

Show 9 quoted lines

> +/**
> + * Appends FETCH_HEAD's merge heads into arr. Returns number of merge heads,
> + * or -1 on error.
> + */
> +static int sha1_array_append_fetch_merge_heads(struct sha1_array *arr)
> +{
> +	int num_heads = 0;
> +	char *filename = git_path("FETCH_HEAD");
> +	FILE *fp = fopen(filename, "r");

I guess this is one instance where we could avoid writting (fetch) and then parsing (here) if we had a better internal API.

But that can come after, of course.

> +}
> +
> +
> +static void argv_array_push_merge_args(struct argv_array *arr,

Bikeshed: you sometimes have two blank lines between functions,
sometimes one. Not sure it's intended.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/