Re: [PATCH/RFC/GSOC] make git-pull a builtin
On Mon, Mar 23, 2015 at 6:18 PM, Duy Nguyen pclo...@gmail.com 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. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH/RFC/GSOC] make git-pull a builtin
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 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH/RFC/GSOC] make git-pull a builtin
On Sat, Mar 21, 2015 at 8:23 PM, Paul Tan pyoka...@gmail.com wrote: 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. Could you share these changes? I'm just wondering if we can add kcov support to the test suite. [1] http://simonkagstrom.github.io/kcov/ [2] http://www.googledrive.com/host/0B4O2AiYulllpfmJlTW4xT050OVVicnNWWS02dm52aTJ2TFIwQ2QwdWh0VHotSkU4eUNNWjg At least we can see [3] that not many options in git-pull is executed. Cool reports. [3] https://83dc3588620c5a97164396ec753a2fa480f6a7b0-www.googledrive.com/host/0B4O2AiYulllpfmJlTW4xT050OVVicnNWWS02dm52aTJ2TFIwQ2QwdWh0VHotSkU4eUNNWjg/git-pull/git-pull.360f32c2.html -- Duy -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH/RFC/GSOC] make git-pull a builtin
Hi, On Sun, Mar 22, 2015 at 1:35 AM, Johannes Schindelin johannes.schinde...@gmx.de wrote: 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 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH/RFC/GSOC] make git-pull a builtin
Hi Paul,
On 2015-03-21 15:00, Paul Tan wrote:
Thanks for your suggestions, I agree with most of them :).
On Wed, Mar 18, 2015 at 5:00 PM, Johannes Schindelin
johannes.schinde...@gmx.de wrote:
+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.
It's used as a callback for the argument parser though, so the
callback signature is required.
Good point ;-)
For readability, I would then suggest to declare `int *result = (int
*)opt-value;` on the top of the function and then use `*result = ...` later.
+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.
Actually, I think that identifying the places where code can be
trivially shared (without requiring major refactoring) should be part
of the project, otherwise lots of code may be duplicated and cause
code bloat. The obvious one would be fork_point() in this patch, which
is copied from the merge-base builtin and not accessible because it
has static linkage. The project should, at the very least, allow the
function to be shared between git-pull and git-merge-base, as well as
to modify the function so that it can fail without die()-ing.
Yes, I agree wholeheartedly. But I like your choice to strike a balance for the
sake of a proof-of-concept.
Ciao,
Johannes
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH/RFC/GSOC] make git-pull a builtin
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 matthieu@grenoble-inp.fr wrote: Paul Tan pyoka...@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. 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 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH/RFC/GSOC] make git-pull a builtin
Hi,
On Thu, Mar 19, 2015 at 6:26 AM, Junio C Hamano gits...@pobox.com wrote:
Paul Tan pyoka...@gmail.com writes:
+/* Global vars since they are used often */
+static char *head_name;
+static const char *head_name_short;
+static unsigned char head_sha1[20];
+static int head_flags;
+
+enum rebase_type {
+ REBASE_FALSE = 0,
+ REBASE_TRUE = 1,
+ REBASE_PRESERVE = 2
+};
+
+/**
+ * Parse rebase config/option value and return corresponding int
+ */
+static int parse_rebase(const char *arg)
+{
+ if (!strcmp(arg, true))
+ return REBASE_TRUE;
+ else if (!strcmp(arg, false))
+ return REBASE_FALSE;
+ else if (!strcmp(arg, preserve))
+ return REBASE_PRESERVE;
+ else
+ return -1; /* Invalid value */
+}
Even though the original does not use bool-or-string-config, we
would want to do the same by doing something like
case (config_maybe_bool()) {
case 0:
return REBASE_FALSE;
case 1:
return REBASE_TRUE;
default:
if (!strcmp(arg, preserve))
return REBASE_PRESERVE;
return -1;
}
and then use that in rebase_config_default().
If you mean letting yes, on, no, off be accepted on the
command line as well, then yes I guess it will be a good idea.
+
+/**
+ * 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);
What happens when neither is defined?
+ strbuf_release(name);
+ if (!value)
+ return REBASE_FALSE;
Hmph, are you sure about this? Isn't this [pull] rebase that does
not have = value, in which case pull.rebase is true?
You cannot use NULL as the sentinel value to tell that you did not
find either branch.*.rebase nor pull.rebase (in which case you want
to default to 'false'). Either of them can be spelled as an
equal-less true, which you will see as value==NULL, and you want to
take that as 'true'.
const char *value = false;
...
if (get_value(..., value))
get_value(..., value));
strbuf_release(name);
if (!value)
return REBASE_TRUE;
return parse_rebase(value);
or something along that line, perhaps?
Whoops, didn't take into account the possibility that the config value
could be NULL. Thanks.
+ 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;
Is value something you need to free before returning from this
function?
From my reading if config.c, the memory of value comes from the
config_set the_config_set (the config cache), so there is no need to
free it.
+static int parse_opt_recurse_submodules(const struct option *opt, const
char *arg, int unset)
+{
+ if (!arg)
+ *(int *)opt-value = unset ? RS_NO : RS_YES;
+ else if (!strcmp(arg, no))
+ *(int *)opt-value = RS_NO;
+ else if (!strcmp(arg, yes))
+ *(int *)opt-value = RS_YES;
+ else if (!strcmp(arg, on-demand))
+ *(int *)opt-value = RS_ON_DEMAND;
+ else
+ return -1;
+ return 0;
I suspect that maybe-bool-or-string comment applies equally here for
the UI consistency.
Yup, I'll keep that in mind.
I'll stop here for now. Thanks.
Thanks.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH/RFC/GSOC] make git-pull a builtin
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 matthieu@grenoble-inp.fr wrote: Paul Tan pyoka...@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. 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 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH/RFC/GSOC] make git-pull a builtin
Hi,
Thanks for your suggestions, I agree with most of them :).
On Wed, Mar 18, 2015 at 5:00 PM, Johannes Schindelin
johannes.schinde...@gmx.de wrote:
+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.
It's used as a callback for the argument parser though, so the
callback signature is required.
+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.
Actually, I think that identifying the places where code can be
trivially shared (without requiring major refactoring) should be part
of the project, otherwise lots of code may be duplicated and cause
code bloat. The obvious one would be fork_point() in this patch, which
is copied from the merge-base builtin and not accessible because it
has static linkage. The project should, at the very least, allow the
function to be shared between git-pull and git-merge-base, as well as
to modify the function so that it can fail without die()-ing.
Regards,
Paul
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH/RFC/GSOC] make git-pull a builtin
Paul Tan writes: I would like to share this very rough prototype with everyone. ... I started this as a just-for-fun exercise to learn about the git internal API I started to rewrite git-pull for similar reasons a couple of months ago, but I haven't had time to complete it. It looks like my version has less work remaining than yours, would you like me to share it? Finally, there is the possibility that in the process of conversion bugs or incompatible behavioral changes may be introduced which are not caught by the test suite. 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. This is why I haven't had time to finish and submit my patch. While the c code is pretty much complete, I felt the test suite needed some extension before I could be confident the new code is correct. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH/RFC/GSOC] make git-pull a builtin
Hi Stephen, On 2015-03-18 09:38, Stephen Robin wrote: Paul Tan writes: I would like to share this very rough prototype with everyone. ... I started this as a just-for-fun exercise to learn about the git internal API I started to rewrite git-pull for similar reasons a couple of months ago, but I haven't had time to complete it. It looks like my version has less work remaining than yours, would you like me to share it? Hmm. It always results in an unnecessary round trip if you ask people to ask you to share it. At this point, because Paul already made his work public, I would say that we should continue with his version. Please understand this as an encouragement for the future to share your code pro-actively, just like Git's own source code has been shared with you without the need for you to request it explicitly. For the record, Duy also already shared his C code to implement `git pull` in C publicly. I think that Paul decided to start again in order to understand every detail of the process of converting a shell script into a builtin; If that was the rationale, I would agree that it is a wise one. Ciao, Johannes -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH/RFC/GSOC] make git-pull a builtin
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
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH/RFC/GSOC] make git-pull a builtin
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 pyoka...@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/
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH/RFC/GSOC] make git-pull a builtin
Paul Tan pyoka...@gmail.com writes:
+/* Global vars since they are used often */
+static char *head_name;
+static const char *head_name_short;
+static unsigned char head_sha1[20];
+static int head_flags;
+
+enum rebase_type {
+ REBASE_FALSE = 0,
+ REBASE_TRUE = 1,
+ REBASE_PRESERVE = 2
+};
+
+/**
+ * Parse rebase config/option value and return corresponding int
+ */
+static int parse_rebase(const char *arg)
+{
+ if (!strcmp(arg, true))
+ return REBASE_TRUE;
+ else if (!strcmp(arg, false))
+ return REBASE_FALSE;
+ else if (!strcmp(arg, preserve))
+ return REBASE_PRESERVE;
+ else
+ return -1; /* Invalid value */
+}
Even though the original does not use bool-or-string-config, we
would want to do the same by doing something like
case (config_maybe_bool()) {
case 0:
return REBASE_FALSE;
case 1:
return REBASE_TRUE;
default:
if (!strcmp(arg, preserve))
return REBASE_PRESERVE;
return -1;
}
and then use that in rebase_config_default().
+
+/**
+ * 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);
What happens when neither is defined?
+ strbuf_release(name);
+ if (!value)
+ return REBASE_FALSE;
Hmph, are you sure about this? Isn't this [pull] rebase that does
not have = value, in which case pull.rebase is true?
You cannot use NULL as the sentinel value to tell that you did not
find either branch.*.rebase nor pull.rebase (in which case you want
to default to 'false'). Either of them can be spelled as an
equal-less true, which you will see as value==NULL, and you want to
take that as 'true'.
const char *value = false;
...
if (get_value(..., value))
get_value(..., value));
strbuf_release(name);
if (!value)
return REBASE_TRUE;
return parse_rebase(value);
or something along that line, perhaps?
+ 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;
Is value something you need to free before returning from this
function?
+static int parse_opt_recurse_submodules(const struct option *opt, const char
*arg, int unset)
+{
+ if (!arg)
+ *(int *)opt-value = unset ? RS_NO : RS_YES;
+ else if (!strcmp(arg, no))
+ *(int *)opt-value = RS_NO;
+ else if (!strcmp(arg, yes))
+ *(int *)opt-value = RS_YES;
+ else if (!strcmp(arg, on-demand))
+ *(int *)opt-value = RS_ON_DEMAND;
+ else
+ return -1;
+ return 0;
I suspect that maybe-bool-or-string comment applies equally here for
the UI consistency.
I'll stop here for now. Thanks.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
