Re: [PATCH] bisect--helper: convert a function in shell to C
On Tue, Mar 22, 2016 at 11:40 AM, Christian Couder wrote: > On Tue, Mar 22, 2016 at 1:28 AM, Stefan Beller wrote: >> On Mon, Mar 21, 2016 at 12:00 PM, Pranit Bauva >> wrote: >>> Convert the code literally without changing its design even though it >>> seems that its obscure as to the use of comparing revision to different >>> bisect >>> arguments which seems like a problem in shell because of the way >>> function arguments are handled. >> >> How would I use the C version instead of the shell version now? > > Hint: one can look at how other functions in builtin/bisect--helper.c are > used. > >> I'd imagine you'd want to change calls in git-bisect.sh from >> check_term_format >> to be: >> git bisect--helper check_term_format > > Hint: to get a good idea of how the call should be, one can look at > the way "git-bisect.sh" already calls "git bisect--helper". > >> and "git bisect--helper" would then call the new static method? >> Once you have the C version working (do we need additional tests >> or can we rely on the test suite being enough for now?), >> you can also delete the shell version. Thanks a lot! I will dig more into this. -- 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] bisect--helper: convert a function in shell to C
On Tue, Mar 22, 2016 at 5:58 AM, Stefan Beller wrote:
> On Mon, Mar 21, 2016 at 12:00 PM, Pranit Bauva wrote:
>> Convert the code literally without changing its design even though it
>> seems that its obscure as to the use of comparing revision to different
>> bisect
>> arguments which seems like a problem in shell because of the way
>> function arguments are handled.
>
> How would I use the C version instead of the shell version now?
> I'd imagine you'd want to change calls in git-bisect.sh from
> check_term_format
> to be:
> git bisect--helper check_term_format
> and "git bisect--helper" would then call the new static method?
> Once you have the C version working (do we need additional tests
> or can we rely on the test suite being enough for now?),
> you can also delete the shell version.
I have to yet think about this. Currently, I just called this function
from cmd_bisect__helper().
> Thanks,
> Stefan
>
>>
>> Signed-off-by: Pranit Bauva
>> ---
>> builtin/bisect--helper.c | 23 +++
>> 1 file changed, 23 insertions(+)
>>
>> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
>> index 3324229..61abe68 100644
>> --- a/builtin/bisect--helper.c
>> +++ b/builtin/bisect--helper.c
>> @@ -2,12 +2,35 @@
>> #include "cache.h"
>> #include "parse-options.h"
>> #include "bisect.h"
>> +#include "refs.h"
>>
>> static const char * const git_bisect_helper_usage[] = {
>> N_("git bisect--helper --next-all [--no-checkout]"),
>> NULL
>> };
>>
>> +static int check_term_format(const char *term, const char *revision, int
>> flag) {
>> + if (check_refname_format(term, flag))
>> + die("'%s' is not a valid term", term);
>> +
>> + if (!strcmp(term, "help") || !strcmp(term, "start") ||
>> + !strcmp(term, "skip") || !strcmp(term, "next") ||
>> + !strcmp(term, "reset") || !strcmp(term, "visualize") ||
>> + !strcmp(term, "replay") || !strcmp(term, "log") ||
>> + !strcmp(term, "run"))
>> + die("can't use the builtin command '%s' as a term", term);
>
> "terms" is missing?
>
> eval_gettext would translate into C as
> die (_("translatable message"));
>
>
>> +
>> + if (!strcmp(term, "bad") || !strcmp(term, "new"))
>> + if(strcmp(revision, "bad"))
>> + die("can't change the meaning of term '%s'", term);
>> +
>> + if (!strcmp(term, "good") || !strcmp(term, "old"))
>> + if (strcmp(revision, "good"))
>> + die("can't change the meaning of term '%s'", term);
>> +
>> + return 1;
>
> Why 1? Usually we use 0 for success in C. die(...) returns with non zero,
> so having 0 here would help us in the shell code to see if the C version
> died (or not).
Sorry I forgot to change the value. I had initially kept it to 0 but
then to do some tweaks, I changed it to 1. I had to do manual tweaks
as I am still scared to use gdb for big projects. I am trying to get
more comfortable with it. Also reading the patch again, I seem to have
forgotten the function declaration statement also.
[1] : http://thread.gmane.org/gmane.comp.version-control.git/289299/focus=289364
--
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] bisect--helper: convert a function in shell to C
On Tue, Mar 22, 2016 at 1:28 AM, Stefan Beller wrote: > On Mon, Mar 21, 2016 at 12:00 PM, Pranit Bauva wrote: >> Convert the code literally without changing its design even though it >> seems that its obscure as to the use of comparing revision to different >> bisect >> arguments which seems like a problem in shell because of the way >> function arguments are handled. > > How would I use the C version instead of the shell version now? Hint: one can look at how other functions in builtin/bisect--helper.c are used. > I'd imagine you'd want to change calls in git-bisect.sh from > check_term_format > to be: > git bisect--helper check_term_format Hint: to get a good idea of how the call should be, one can look at the way "git-bisect.sh" already calls "git bisect--helper". > and "git bisect--helper" would then call the new static method? > Once you have the C version working (do we need additional tests > or can we rely on the test suite being enough for now?), > you can also delete the shell version. -- 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] bisect--helper: convert a function in shell to C
On Mon, Mar 21, 2016 at 12:00 PM, Pranit Bauva wrote:
> Convert the code literally without changing its design even though it
> seems that its obscure as to the use of comparing revision to different bisect
> arguments which seems like a problem in shell because of the way
> function arguments are handled.
How would I use the C version instead of the shell version now?
I'd imagine you'd want to change calls in git-bisect.sh from
check_term_format
to be:
git bisect--helper check_term_format
and "git bisect--helper" would then call the new static method?
Once you have the C version working (do we need additional tests
or can we rely on the test suite being enough for now?),
you can also delete the shell version.
Thanks,
Stefan
>
> Signed-off-by: Pranit Bauva
> ---
> builtin/bisect--helper.c | 23 +++
> 1 file changed, 23 insertions(+)
>
> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
> index 3324229..61abe68 100644
> --- a/builtin/bisect--helper.c
> +++ b/builtin/bisect--helper.c
> @@ -2,12 +2,35 @@
> #include "cache.h"
> #include "parse-options.h"
> #include "bisect.h"
> +#include "refs.h"
>
> static const char * const git_bisect_helper_usage[] = {
> N_("git bisect--helper --next-all [--no-checkout]"),
> NULL
> };
>
> +static int check_term_format(const char *term, const char *revision, int
> flag) {
> + if (check_refname_format(term, flag))
> + die("'%s' is not a valid term", term);
> +
> + if (!strcmp(term, "help") || !strcmp(term, "start") ||
> + !strcmp(term, "skip") || !strcmp(term, "next") ||
> + !strcmp(term, "reset") || !strcmp(term, "visualize") ||
> + !strcmp(term, "replay") || !strcmp(term, "log") ||
> + !strcmp(term, "run"))
> + die("can't use the builtin command '%s' as a term", term);
"terms" is missing?
eval_gettext would translate into C as
die (_("translatable message"));
> +
> + if (!strcmp(term, "bad") || !strcmp(term, "new"))
> + if(strcmp(revision, "bad"))
> + die("can't change the meaning of term '%s'", term);
> +
> + if (!strcmp(term, "good") || !strcmp(term, "old"))
> + if (strcmp(revision, "good"))
> + die("can't change the meaning of term '%s'", term);
> +
> + return 1;
Why 1? Usually we use 0 for success in C. die(...) returns with non zero,
so having 0 here would help us in the shell code to see if the C version
died (or not).
> +}
> +
> int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
> {
> int next_all = 0;
>
> --
> https://github.com/git/git/pull/216
> --
> 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
--
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
