From: Matthieu Moy <Matthieu.Moy@grenoble-inp.fr>
To: Paul Tan <pyokagan@gmail.com>
Cc: git@vger.kernel.org,
Johannes Schindelin <johannes.schindelin@gmx.de>,
Duy Nguyen <pclouds@gmail.com>
Subject: Re: [PATCH/RFC/GSOC] make git-pull a builtin
Date: Wed, 18 Mar 2015 18:52:30 +0100 [thread overview]
Message-ID: <vpqwq2eyyzl.fsf@anie.imag.fr> (raw)
In-Reply-To: <1426600662-32276-1-git-send-email-pyokagan@gmail.com> (Paul Tan's message of "Tue, 17 Mar 2015 21:57:42 +0800")
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.
> 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/
next prev parent reply other threads:[~2015-03-18 17:52 UTC|newest]
Thread overview: 15+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-03-17 13:57 [PATCH/RFC/GSOC] make git-pull a builtin Paul Tan
2015-03-18 8:38 ` Stephen Robin
2015-03-18 9:24 ` Johannes Schindelin
2015-03-18 9:00 ` Johannes Schindelin
2015-03-21 14:00 ` Paul Tan
2015-03-21 17:27 ` Johannes Schindelin
2015-03-18 17:52 ` Matthieu Moy [this message]
2015-03-21 13:23 ` Paul Tan
2015-03-21 17:35 ` Johannes Schindelin
2015-03-22 17:39 ` Paul Tan
2015-03-23 9:07 ` Johannes Schindelin
2015-03-23 10:18 ` Duy Nguyen
2015-03-24 15:58 ` Paul Tan
2015-03-18 22:26 ` Junio C Hamano
2015-03-21 13:40 ` Paul Tan
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=vpqwq2eyyzl.fsf@anie.imag.fr \
--to=matthieu.moy@grenoble-inp.fr \
--cc=git@vger.kernel.org \
--cc=johannes.schindelin@gmx.de \
--cc=pclouds@gmail.com \
--cc=pyokagan@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).