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 [off-list ref] 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.
quoted hunk
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/
Hi,
Thanks for the review, though I would like to work on the proposal now
before the deadline passes :)
On Thu, Mar 19, 2015 at 1:52 AM, Matthieu Moy
[off-list ref] wrote:
Paul Tan [off-list ref] writes:
quoted
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.
Maybe code coverage tools could help here so we only need to focus on
the code paths that are untested by the test suite. At the minimum,
all of the non-trivial code paths in both the shell script and the
converted builtin must be covered by tests. This should help to
eliminate most sources of breakages. Anything further than that would
require an experienced understanding of all the possible important
inputs to be tested, which I personally feel would make the project
quite tedious.
I see git already has gcov support. For shell scripts, maybe kcov[1]
could be used. With some slight code changes, I managed to generate a
report for the git-pull tests[2] which should at least provide a good
starting point for how the tests can be improved.
[1] http://simonkagstrom.github.io/kcov/
[2] <http://www.googledrive.com/host/0B4O2AiYulllpfmJlTW4xT050OVVicnNWWS02dm52aTJ2TFIwQ2QwdWh0VHotSkU4eUNNWjg>
Regards,
Paul
Hi Paul,
On 2015-03-21 14:23, Paul Tan wrote:
Thanks for the review, though I would like to work on the proposal now
before the deadline passes :)
That makes sense.
On Thu, Mar 19, 2015 at 1:52 AM, Matthieu Moy
[off-list ref] wrote:
quoted
Paul Tan [off-list ref] writes:
quoted
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.
Maybe code coverage tools could help here so we only need to focus on
the code paths that are untested by the test suite. At the minimum,
all of the non-trivial code paths in both the shell script and the
converted builtin must be covered by tests. This should help to
eliminate most sources of breakages. Anything further than that would
require an experienced understanding of all the possible important
inputs to be tested, which I personally feel would make the project
quite tedious.
I see git already has gcov support. For shell scripts, maybe kcov[1]
could be used. With some slight code changes, I managed to generate a
report for the git-pull tests[2] which should at least provide a good
starting point for how the tests can be improved.
While it is often a tempting idea to make test suites as thorough as possible, there lies a true danger herein. True war story: in one of the projects I was involved in, the test suite grew to a size that one complete run lasted two weeks. Yes, that is fourteen days. Needless to say: this test suite was run rarely. How useful is a test suite that is run rarely? More useful than a non-existent one, to be sure, but it is still more of a burden than a boon.
Now, on Windows the test suite takes almost three hours to run. This really, really slows down development.
So while we are not yet at the "too large to be useful state", I would caution against trying to get there.
Instead, I would really like to focus on the *usage*. Calling `git grep "git pull" t/` should give you an idea what usage of `git pull` is already tested. It should be pretty easy to come up with a list of *common* use cases, and if any of them are not covered, adding tests for them is simple and straight-forward, too.
Ciao,
Johannes
Hi,
On Sun, Mar 22, 2015 at 1:35 AM, Johannes Schindelin
[off-list ref] wrote:
quoted
Maybe code coverage tools could help here so we only need to focus on
the code paths that are untested by the test suite. At the minimum,
all of the non-trivial code paths in both the shell script and the
converted builtin must be covered by tests. This should help to
eliminate most sources of breakages. Anything further than that would
require an experienced understanding of all the possible important
inputs to be tested, which I personally feel would make the project
quite tedious.
I see git already has gcov support. For shell scripts, maybe kcov[1]
could be used. With some slight code changes, I managed to generate a
report for the git-pull tests[2] which should at least provide a good
starting point for how the tests can be improved.
While it is often a tempting idea to make test suites as thorough as possible, there lies a true danger herein. True war story: in one of the projects I was involved in, the test suite grew to a size that one complete run lasted two weeks. Yes, that is fourteen days. Needless to say: this test suite was run rarely. How useful is a test suite that is run rarely? More useful than a non-existent one, to be sure, but it is still more of a burden than a boon.
Now, on Windows the test suite takes almost three hours to run. This really, really slows down development.
So while we are not yet at the "too large to be useful state", I would caution against trying to get there.
Instead, I would really like to focus on the *usage*. Calling `git grep "git pull" t/` should give you an idea what usage of `git pull` is already tested. It should be pretty easy to come up with a list of *common* use cases, and if any of them are not covered, adding tests for them is simple and straight-forward, too.
The code coverage tools can help here as well. The kcov output clearly
shows which options of git-pull are currently not being tested. But
yes, I agree that the test suite shouldn't be relied too much on
compared to code inspection and review.
On another important topic, though, along with git-pull.sh, I'm
looking for another script to convert in parallel with git-pull.sh so
that there will be no blocks due to patch review. Generally, I think
rewriting scripts that are called frequently by users, or spawn a lot
of processes due to loops, would be most desirable because the runtime
gains would be much higher. A quick review of the scripts shows that
git-am.sh, git-rebase--interactive.sh and git-quiltimport.sh have
pretty heavy loops with lots of process spawning that grows with
input.
I'm currently leaning with git-am because not only is it a frequently
used command, git-rebase--am.sh (for non-interactive rebase) calls it
as well. In fact, quick tests show that it takes up 98% of
git-rebase's execution time on Windows, so if git-am's performance
improves it would be a huge win on many fronts. git-am's code also
seems to be manageable for a 3-month project.
Anyway, I would like to know if you (or anyone else) have any scripts in mind.
(I also think that just 2 scripts would be enough to fill the 3
months, but that might be me just being too conservative)
Regards,
Paul
Hi Paul,
On 2015-03-22 18:39, Paul Tan wrote:
The code coverage tools can help here as well. The kcov output clearly
shows which options of git-pull are currently not being tested. But
yes, I agree that the test suite shouldn't be relied too much on
compared to code inspection and review.
Fully agree.
On another important topic, though, along with git-pull.sh, I'm
looking for another script to convert in parallel with git-pull.sh so
that there will be no blocks due to patch review. Generally, I think
rewriting scripts that are called frequently by users, or spawn a lot
of processes due to loops, would be most desirable because the runtime
gains would be much higher. A quick review of the scripts shows that
git-am.sh, git-rebase--interactive.sh and git-quiltimport.sh have
pretty heavy loops with lots of process spawning that grows with
input.
I'm currently leaning with git-am because not only is it a frequently
used command, git-rebase--am.sh (for non-interactive rebase) calls it
as well. In fact, quick tests show that it takes up 98% of
git-rebase's execution time on Windows, so if git-am's performance
improves it would be a huge win on many fronts. git-am's code also
seems to be manageable for a 3-month project.
Yeah, `git am` is definitely a good pick.
Thanks!
Johannes
On Mon, Mar 23, 2015 at 6:18 PM, Duy Nguyen [off-list ref] wrote:
Could you share these changes? I'm just wondering if we can add kcov
support to the test suite.
In this case it's more of an embarrassing hack, as I just needed a way
to make git run "kcov outdir git-pull.sh" whenever git pull is called
since kcov will not instrument any spawned subprocesses. I modified
execv_dashed_external() in git.c to prepend kcov to argv (diff
probably munged by gmail):
diff --git a/git.c b/git.c
index 8c7ee9c..0f8e7d4 100644
--- a/git.c
+++ b/git.c
@@ -536,6 +536,8 @@ static void execv_dashed_external(const char **argv)
struct strbuf cmd = STRBUF_INIT;
const char *tmp;
int status;
+ struct argv_array args = ARGV_ARRAY_INIT;
+ int i;
if (use_pager == -1)
use_pager = check_pager_config(argv[0]);@@ -551,6 +553,11 @@ static void execv_dashed_external(const char **argv)
*/
tmp = argv[0];
argv[0] = cmd.buf;
+ argv_array_push(&args, "kcov");
+ argv_array_push(&args, "/home/pyokagan/pyk/git/kcov");
+ argv_array_push(&args, cmd.buf);
+ for (i = 1; argv[i]; i++)
+ argv_array_push(&args, argv[i]);
trace_argv_printf(argv, "trace: exec:");
@@ -558,7 +565,7 @@ static void execv_dashed_external(const char **argv)
* if we fail because the command is not found, it is
* OK to return. Otherwise, we just pass along the status code.
*/
- status = run_command_v_opt(argv, RUN_SILENT_EXEC_FAILURE |RUN_CLEAN_ON_EXIT);
+ status = run_command_v_opt(args.argv, RUN_SILENT_EXEC_FAILURE
| RUN_CLEAN_ON_EXIT);
if (status >= 0 || errno != ENOENT)
exit(status);
I'm guessing a real solution is to follow what the test suite does for
the --valgrind option, though I haven't looked into it in detail.