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@xxxxxxxxx> 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.
> 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.
> +/**
> + * 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/
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html