Re: [PATCH] bisect--helper: convert a function in shell to C

[Pranit Bauva <pranit.bauva@gmail.com>]

On Tue, Mar 22, 2016 at 5:58 AM, Stefan Beller <sbeller@google.com> wrote:
> On Mon, Mar 21, 2016 at 12:00 PM, Pranit Bauva <pranit.bauva@gmail.com> 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 <term> <bad/new>
> to be:
>     git bisect--helper check_term_format <term> <bad/new>
> 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 <pranit.bauva@gmail.com>
>> ---
>>  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