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

classic Classic list List threaded Threaded
112 messages Options
1234 ... 6
Reply | Threaded
Open this post in threaded view
|

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

pranitbauva1997
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.

Signed-off-by: Pranit Bauva <[hidden email]>
---
 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);
+
+ 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;
+}
+
 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 [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

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

Stefan Beller-4
On Mon, Mar 21, 2016 at 12:00 PM, Pranit Bauva <[hidden email]> 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.

Thanks,
Stefan

>
> Signed-off-by: Pranit Bauva <[hidden email]>
> ---
>  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 [hidden email]
> 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 [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

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

Christian Couder-2
On Tue, Mar 22, 2016 at 1:28 AM, Stefan Beller <[hidden email]> wrote:
> On Mon, Mar 21, 2016 at 12:00 PM, Pranit Bauva <[hidden email]> 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 <term> <bad/new>
> to be:
>     git bisect--helper check_term_format <term> <bad/new>

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 [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

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

pranitbauva1997
In reply to this post by Stefan Beller-4
On Tue, Mar 22, 2016 at 5:58 AM, Stefan Beller <[hidden email]> wrote:

> On Mon, Mar 21, 2016 at 12:00 PM, Pranit Bauva <[hidden email]> 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 <[hidden email]>
>> ---
>>  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 [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

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

pranitbauva1997
In reply to this post by Christian Couder-2
On Tue, Mar 22, 2016 at 11:40 AM, Christian Couder
<[hidden email]> wrote:

> On Tue, Mar 22, 2016 at 1:28 AM, Stefan Beller <[hidden email]> wrote:
>> On Mon, Mar 21, 2016 at 12:00 PM, Pranit Bauva <[hidden email]> 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 <term> <bad/new>
>> to be:
>>     git bisect--helper check_term_format <term> <bad/new>
>
> 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 [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

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

pranitbauva1997
In reply to this post by pranitbauva1997
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.

The argument handling is kind of hard coded right now because it is not
really be meant to be used like this and this is just for testing
purposes whether this new method is as functional as its counter part.
The shell counter part of the method has been retained for historical
purposes.

Signed-off-by: Pranit Bauva <[hidden email]>
---
 builtin/bisect--helper.c | 37 +++++++++++++++++++++++++++++++++++++
 git-bisect.sh            |  4 ++--
 2 files changed, 39 insertions(+), 2 deletions(-)

diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
index 3324229..6cdae82 100644
--- a/builtin/bisect--helper.c
+++ b/builtin/bisect--helper.c
@@ -2,27 +2,64 @@
 #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]"),
+ N_("git bisect--helper --check-term-format <term> <revision>"),
  NULL
 };
 
+static int check_term_format(const char *term, const char *revision, int flags);
+
+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);
+
+ 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 0;
+}
+
 int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
 {
  int next_all = 0;
  int no_checkout = 0;
+ const char *term;
  struct option options[] = {
  OPT_BOOL(0, "next-all", &next_all,
  N_("perform 'git bisect next'")),
  OPT_BOOL(0, "no-checkout", &no_checkout,
  N_("update BISECT_HEAD instead of checking out the current commit")),
+ OPT_STRING(0, "check-term-format", &term, N_("term"),
+ N_("check the format of the ref")),
  OPT_END()
  };
 
  argc = parse_options(argc, argv, prefix, options,
      git_bisect_helper_usage, 0);
 
+
+ if (term != NULL) {
+ if (argc > 0)
+ return check_term_format(term, argv[0], 0);
+ else
+ die("no revision provided with check_for_term");
+ }
+
  if (!next_all)
  usage_with_options(git_bisect_helper_usage, options);
 
diff --git a/git-bisect.sh b/git-bisect.sh
index 5d1cb00..ea237be 100755
--- a/git-bisect.sh
+++ b/git-bisect.sh
@@ -564,8 +564,8 @@ write_terms () {
  then
  die "$(gettext "please use two different terms")"
  fi
- check_term_format "$TERM_BAD" bad
- check_term_format "$TERM_GOOD" good
+ git bisect--helper --check-term-format="$TERM_BAD" bad
+ git bisect--helper --check-term-format="$TERM_GOOD" good
  printf '%s\n%s\n' "$TERM_BAD" "$TERM_GOOD" >"$GIT_DIR/BISECT_TERMS"
 }
 

--
https://github.com/git/git/pull/216
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

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

Johannes Schindelin
Hi,

On Tue, 22 Mar 2016, 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

s/its/it is/

> bisect arguments which seems like a problem in shell because of the way
> function arguments are handled.

I agree that it is obscure. That is why I would suggest to fix it during
the conversion. Using 'new_term' and 'orig_term' (or something similar)
would make much more sense.

Another good idea would be to include the shell code, or at least to
provide a link such as:

        https://github.com/git/git/blob/v2.8.0-rc4/git-bisect.sh#L572-L597

> The argument handling is kind of hard coded right now because it is not
> really be meant to be used like this and this is just for testing
> purposes whether this new method is as functional as its counter part.
> The shell counter part of the method has been retained for historical
> purposes.

Still, it would make more sense (both in terms of readability and in terms
of code safety) to introduce and use a function like

static int one_of(const char *term, ...)
{
        va_list matches;
        const char *match;

        va_start(matches, term);
        while ((match = va_arg(matches, const char *)))
                if (!strcmp(term, match))
                        return 1;
        va_end(matches);

        return 0;
}

> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
> index 3324229..6cdae82 100644
> --- a/builtin/bisect--helper.c
> +++ b/builtin/bisect--helper.c
> @@ -2,27 +2,64 @@
>  #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]"),
> + N_("git bisect--helper --check-term-format <term> <revision>"),

Good. This imitates the existing code.

>   NULL
>  };
>  
> +static int check_term_format(const char *term, const char *revision, int flags);
> +
> +static int check_term_format(const char *term, const char *revision, int flag) {

Since you define the check_term_format() function here, the declaration
above is unnecessary. Let's just delete it.

> + 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);

This would look so much nicer using the one_of() function above.

Please also note that our coding convention (as can be seen from the
existing code in builtin/*.c) is to indent the condition differently than
the block, either using an extra tab, or by using 4 spaces instead of the
tab.

> + 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);

These two can be combined. Actually, these *four* can easily be combined:

        if ((one_of(term, "bad", "new", NULL) && strcmp(orig, "bad")) ||
            (one_of(term, "good", "old", NULL) && strcmp(orig, "good")))
                die("can't change the meaning of term '%s'", term);

>  int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
>  {
>   int next_all = 0;
>   int no_checkout = 0;
> + const char *term;

Better use the existing convention:

        int check_term_format = 0;

>   struct option options[] = {
>   OPT_BOOL(0, "next-all", &next_all,
>   N_("perform 'git bisect next'")),
>   OPT_BOOL(0, "no-checkout", &no_checkout,
>   N_("update BISECT_HEAD instead of checking out the current commit")),
> + OPT_STRING(0, "check-term-format", &term, N_("term"),
> + N_("check the format of the ref")),

Hmm. The existing code suggests to use OPT_BOOL instead.

>   OPT_END()
>   };
>  
>   argc = parse_options(argc, argv, prefix, options,
>       git_bisect_helper_usage, 0);
>  
> +
> + if (term != NULL) {
> + if (argc > 0)

Here you need to test for a precise argc, not for a range.

> + return check_term_format(term, argv[0], 0);
> + else
> + die("no revision provided with check_for_term");
> + }
> +
>   if (!next_all)
>   usage_with_options(git_bisect_helper_usage, options);
>  
> diff --git a/git-bisect.sh b/git-bisect.sh
> index 5d1cb00..ea237be 100755
> --- a/git-bisect.sh
> +++ b/git-bisect.sh
> @@ -564,8 +564,8 @@ write_terms () {
>   then
>   die "$(gettext "please use two different terms")"
>   fi
> - check_term_format "$TERM_BAD" bad
> - check_term_format "$TERM_GOOD" good
> + git bisect--helper --check-term-format="$TERM_BAD" bad
> + git bisect--helper --check-term-format="$TERM_GOOD" good

The existing convention is to make the first argument *not* a value of the
"option", i.e. `--check-term-format "$TERM_BAD"` without an equal sign.

Did you also run the test suite after compiling, to verify that the
documented expectations are still met after the conversion?

Ciao,
Johannes
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

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

Johannes Schindelin
Hi,

On Tue, 22 Mar 2016, Johannes Schindelin wrote:

> On Tue, 22 Mar 2016, Pranit Bauva wrote:
>
> > + 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);
>
> These two can be combined. Actually, these *four* can easily be combined:
>
> if ((one_of(term, "bad", "new", NULL) && strcmp(orig, "bad")) ||
>    (one_of(term, "good", "old", NULL) && strcmp(orig, "good")))
> die("can't change the meaning of term '%s'", term);

Completely forgot to mention: This conversion skipped the comment

        # In theory, nothing prevents swapping
        # completely good and bad, but this situation
        # could be confusing and hasn't been tested
        # enough. Forbid it for now.

Let's port that comment over, too?

Ciao,
Johannes
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

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

Junio C Hamano
In reply to this post by Johannes Schindelin
Johannes Schindelin <[hidden email]> writes:

> static int one_of(const char *term, ...)
> {
> va_list matches;
> const char *match;
>
> va_start(matches, term);
> while ((match = va_arg(matches, const char *)))
> if (!strcmp(term, match))
> return 1;
> va_end(matches);
>
> return 0;
> }

This is a very good suggestion.  We tend to avoid explicit
comparison to NULL and zero, but we avoid assignment in condition
part of if/while statements even more, so

  while ((match = va_arg(matches, const char *)) != NULL)

would be the best compromise to make it clear and readable.

>> + 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);
>
> This would look so much nicer using the one_of() function above.
>
> Please also note that our coding convention (as can be seen from the
> existing code in builtin/*.c) is to indent the condition differently than
> the block, either using an extra tab, or by using 4 spaces instead of the
> tab.

In general, "or by using 4 spaces" is better spelled as "or by
indenting so that the line aligns with the beginning of the inside
of the opening parenthesis on the above line"; "if (" happens to
take 4 display spaces and that is where "4" comes from and it would
be different for "while (" condition ;-).

But with one_of(...) this part would change a lot, I imagine.

>>   struct option options[] = {
>>   OPT_BOOL(0, "next-all", &next_all,
>>   N_("perform 'git bisect next'")),
>>   OPT_BOOL(0, "no-checkout", &no_checkout,
>>   N_("update BISECT_HEAD instead of checking out the current commit")),
>> + OPT_STRING(0, "check-term-format", &term, N_("term"),
>> + N_("check the format of the ref")),
>
> Hmm. The existing code suggests to use OPT_BOOL instead.
> ...
> The existing convention is to make the first argument *not* a value of the
> "option", i.e. `--check-term-format "$TERM_BAD"` without an equal sign.

I think it is preferrable to keep using OPT_BOOL() for this new one
if we are incrementally building on top of existing code.

But if the convention is that the option is to specify what opration
is invoked, using OPT_CMDMODE() to implement all of them would be a
worthwhile cleanup to consider at some point.

I agree with all the points you raised in your review.  Just wanted
to add some clarification myself.

Thanks.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

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

Johannes Schindelin
Hi Junio,

On Tue, 22 Mar 2016, Junio C Hamano wrote:

> Johannes Schindelin <[hidden email]> writes:
>
> >>   struct option options[] = {
> >>   OPT_BOOL(0, "next-all", &next_all,
> >>   N_("perform 'git bisect next'")),
> >>   OPT_BOOL(0, "no-checkout", &no_checkout,
> >>   N_("update BISECT_HEAD instead of checking out the current commit")),
> >> + OPT_STRING(0, "check-term-format", &term, N_("term"),
> >> + N_("check the format of the ref")),
> >
> > Hmm. The existing code suggests to use OPT_BOOL instead.
> > ...
> > The existing convention is to make the first argument *not* a value of the
> > "option", i.e. `--check-term-format "$TERM_BAD"` without an equal sign.
>
> I think it is preferrable to keep using OPT_BOOL() for this new one
> if we are incrementally building on top of existing code.
>
> But if the convention is that the option is to specify what opration
> is invoked, using OPT_CMDMODE() to implement all of them would be a
> worthwhile cleanup to consider at some point.

Good point, I keep forgetting that OPT_CMDMODE() was introduced
specifically for subcommands.

Ciao,
Dscho
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

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

pranitbauva1997
In reply to this post by Johannes Schindelin
On Tue, Mar 22, 2016 at 8:39 PM, Johannes Schindelin
<[hidden email]> wrote:
> Hi,
>
> On Tue, 22 Mar 2016, 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
>
> s/its/it is/

Sure! A typo.

>> bisect arguments which seems like a problem in shell because of the way
>> function arguments are handled.
>
> I agree that it is obscure. That is why I would suggest to fix it during
> the conversion. Using 'new_term' and 'orig_term' (or something similar)
> would make much more sense.
>
> Another good idea would be to include the shell code, or at least to
> provide a link such as:
>
>         https://github.com/git/git/blob/v2.8.0-rc4/git-bisect.sh#L572-L597
>

I will take care about this henceforth.

>> The argument handling is kind of hard coded right now because it is not
>> really be meant to be used like this and this is just for testing
>> purposes whether this new method is as functional as its counter part.
>> The shell counter part of the method has been retained for historical
>> purposes.
>
> Still, it would make more sense (both in terms of readability and in terms
> of code safety) to introduce and use a function like
>
> static int one_of(const char *term, ...)
> {
>         va_list matches;
>         const char *match;
>
>         va_start(matches, term);
>         while ((match = va_arg(matches, const char *)))
>                 if (!strcmp(term, match))
>                         return 1;
>         va_end(matches);
>
>         return 0;
> }
>

>> +static int check_term_format(const char *term, const char *revision, int flags);
>> +
>> +static int check_term_format(const char *term, const char *revision, int flag) {
>
> Since you define the check_term_format() function here, the declaration
> above is unnecessary. Let's just delete it.

Yes. We could just add functions below this so that it would not
create a problem.

>
>> +     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);
>
> This would look so much nicer using the one_of() function above.

one_of() will definitely make it clean.

> Please also note that our coding convention (as can be seen from the
> existing code in builtin/*.c) is to indent the condition differently than
> the block, either using an extra tab, or by using 4 spaces instead of the
> tab.
>

I

>> +     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);
>
> These two can be combined. Actually, these *four* can easily be combined:
>
>         if ((one_of(term, "bad", "new", NULL) && strcmp(orig, "bad")) ||
>             (one_of(term, "good", "old", NULL) && strcmp(orig, "good")))
>                 die("can't change the meaning of term '%s'", term);
>
>>  int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
>>  {
>>       int next_all = 0;
>>       int no_checkout = 0;
>> +     const char *term;
>
> Better use the existing convention:
>
>         int check_term_format = 0;
>
>>       struct option options[] = {
>>               OPT_BOOL(0, "next-all", &next_all,
>>                        N_("perform 'git bisect next'")),
>>               OPT_BOOL(0, "no-checkout", &no_checkout,
>>                        N_("update BISECT_HEAD instead of checking out the current commit")),
>> +             OPT_STRING(0, "check-term-format", &term, N_("term"),
>> +                      N_("check the format of the ref")),
>
> Hmm. The existing code suggests to use OPT_BOOL instead.
>
>>               OPT_END()
>>       };
>>
>>       argc = parse_options(argc, argv, prefix, options,
>>                            git_bisect_helper_usage, 0);
>>
>> +
>> +     if (term != NULL) {
>> +             if (argc > 0)
>
> Here you need to test for a precise argc, not for a range.

True.

>> +                     return check_term_format(term, argv[0], 0);
>> +             else
>> +                     die("no revision provided with check_for_term");
>> +     }
>> +
>>       if (!next_all)
>>               usage_with_options(git_bisect_helper_usage, options);
>>
>> diff --git a/git-bisect.sh b/git-bisect.sh
>> index 5d1cb00..ea237be 100755
>> --- a/git-bisect.sh
>> +++ b/git-bisect.sh
>> @@ -564,8 +564,8 @@ write_terms () {
>>       then
>>               die "$(gettext "please use two different terms")"
>>       fi
>> -     check_term_format "$TERM_BAD" bad
>> -     check_term_format "$TERM_GOOD" good
>> +     git bisect--helper --check-term-format="$TERM_BAD" bad
>> +     git bisect--helper --check-term-format="$TERM_GOOD" good
>
> The existing convention is to make the first argument *not* a value of the
> "option", i.e. `--check-term-format "$TERM_BAD"` without an equal sign.

Will change this.

> Did you also run the test suite after compiling, to verify that the
> documented expectations are still met after the conversion?

Yes I did run the tests. They produce the same results as they did before.
To ease review I will next time include these the output of the tests
in the commented section.

t6002-rev-list-bisect.sh : http://paste.ubuntu.com/15473728/
t6030-bisect-porcelain.sh : http://paste.ubuntu.com/15473734/
t6041-bisect-submodule.sh : http://paste.ubuntu.com/15473743/

Is there any other test I would need to run?

Regards,
Pranit Bauva
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

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

pranitbauva1997
In reply to this post by Johannes Schindelin
On Tue, Mar 22, 2016 at 8:41 PM, Johannes Schindelin
<[hidden email]> wrote:

> Hi,
>
> On Tue, 22 Mar 2016, Johannes Schindelin wrote:
>
>> On Tue, 22 Mar 2016, Pranit Bauva wrote:
>>
>> > +   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);
>>
>> These two can be combined. Actually, these *four* can easily be combined:
>>
>>       if ((one_of(term, "bad", "new", NULL) && strcmp(orig, "bad")) ||
>>           (one_of(term, "good", "old", NULL) && strcmp(orig, "good")))
>>               die("can't change the meaning of term '%s'", term);
>
> Completely forgot to mention: This conversion skipped the comment
>
>         # In theory, nothing prevents swapping
>         # completely good and bad, but this situation
>         # could be confusing and hasn't been tested
>         # enough. Forbid it for now.
>
> Let's port that comment over, too?

Sure! Adding a comment won't harm anyone. We can remove it when its
thoroughly tested.
>
> Ciao,
> Johannes
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

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

pranitbauva1997
In reply to this post by Junio C Hamano
On Tue, Mar 22, 2016 at 9:33 PM, Junio C Hamano <[hidden email]> wrote:

> Johannes Schindelin <[hidden email]> writes:
>
>> static int one_of(const char *term, ...)
>> {
>>       va_list matches;
>>       const char *match;
>>
>>       va_start(matches, term);
>>       while ((match = va_arg(matches, const char *)))
>>               if (!strcmp(term, match))
>>                       return 1;
>>       va_end(matches);
>>
>>       return 0;
>> }
>
> This is a very good suggestion.  We tend to avoid explicit
> comparison to NULL and zero, but we avoid assignment in condition
> part of if/while statements even more, so
>
>         while ((match = va_arg(matches, const char *)) != NULL)
>
> would be the best compromise to make it clear and readable.
>
>>> +    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);
>>
>> This would look so much nicer using the one_of() function above.
>>
>> Please also note that our coding convention (as can be seen from the
>> existing code in builtin/*.c) is to indent the condition differently than
>> the block, either using an extra tab, or by using 4 spaces instead of the
>> tab.
>
> In general, "or by using 4 spaces" is better spelled as "or by
> indenting so that the line aligns with the beginning of the inside
> of the opening parenthesis on the above line"; "if (" happens to
> take 4 display spaces and that is where "4" comes from and it would
> be different for "while (" condition ;-).

I will definitely keep this in mind henceforth.

>
> But with one_of(...) this part would change a lot, I imagine.
>>>      struct option options[] = {
>>>              OPT_BOOL(0, "next-all", &next_all,
>>>                       N_("perform 'git bisect next'")),
>>>              OPT_BOOL(0, "no-checkout", &no_checkout,
>>>                       N_("update BISECT_HEAD instead of checking out the current commit")),
>>> +            OPT_STRING(0, "check-term-format", &term, N_("term"),
>>> +                     N_("check the format of the ref")),
>>
>> Hmm. The existing code suggests to use OPT_BOOL instead.
>> ...
>> The existing convention is to make the first argument *not* a value of the
>> "option", i.e. `--check-term-format "$TERM_BAD"` without an equal sign.
>
> I think it is preferrable to keep using OPT_BOOL() for this new one
> if we are incrementally building on top of existing code.
>
> But if the convention is that the option is to specify what opration
> is invoked, using OPT_CMDMODE() to implement all of them would be a
> worthwhile cleanup to consider at some point.

OPT_CMDMODE() is actually a better option. I also noticed that it
isn't mentioned in Documentation/technical/api-parse-options.txt .
Should I send a patch to include it there just to make it easier for
someone who is new and isn't aware of the changes ?

> I agree with all the points you raised in your review.  Just wanted
> to add some clarification myself.
>
> Thanks.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

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

Stefan Beller-4
On Tue, Mar 22, 2016 at 10:52 AM, Pranit Bauva <[hidden email]> wrote:
> OPT_CMDMODE() is actually a better option. I also noticed that it
> isn't mentioned in Documentation/technical/api-parse-options.txt .
> Should I send a patch to include it there just to make it easier for
> someone who is new and isn't aware of the changes ?

Patches to outdated documentation are most awesome. ;)
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

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

pranitbauva1997
In reply to this post by pranitbauva1997
Convert the code literally without changing its design even though it
seems that it is 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.

The argument handling is kind of hard coded right now because it is not
really be meant to be used like this and this is just for testing
purposes whether this new method is as functional as its counter part.
The shell counter part of the method has been retained for historical
purposes.

Also using OPT_CMDMODE() to handle check-term-format and next-all
because these sub commands are independent and error should be shown if used
together and should be handled independently.

This commit reduces the number of failed tests in
t6030-bisect-porcelain.sh and t6041-bisect-submodule.sh

The corresponding shell function is :
https://github.com/git/git/blob/v2.8.0-rc4/git-bisect.sh#L572-L597

Signed-off-by: Pranit Bauva <[hidden email]>

---
Changes wrt v1:
 - Remove the function declaration
 - Introduce another method one_of() to reduce the clutter in if
 - Add the documentation as to which part should remain untouched
 - Use OPT_CMDMODE() for --check-term-format and --next-all
 - Remove the '=' in git-bisect.sh
 - Respect the coding convention to indent when a line is spread across
   many lines
 - s/its/it is/g
 - Output of tests:
   - t6002 : http://paste.ubuntu.com/15477883/
   - t6030 : http://paste.ubuntu.com/15477887/
   - t6041 : http://paste.ubuntu.com/15477897/
 - Add the comment that a part shouldn't be touched
---
 builtin/bisect--helper.c | 73 ++++++++++++++++++++++++++++++++++++++++++++----
 git-bisect.sh            |  4 +--
 2 files changed, 70 insertions(+), 7 deletions(-)

diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
index 3324229..ab3891c 100644
--- a/builtin/bisect--helper.c
+++ b/builtin/bisect--helper.c
@@ -2,30 +2,93 @@
 #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]"),
+ N_("git bisect--helper --check-term-format <term> <revision>"),
  NULL
 };
 
+static int one_of(const char *term, ...) {
+ va_list matches;
+ const char *match;
+
+ va_start(matches, term);
+ while ((match = va_arg(matches, const char *)) != NULL)
+ if (!strcmp(term, match))
+ return 1;
+
+ va_end(matches);
+
+ return 0;
+}
+
+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 (one_of(term, "help", "start", "skip", "next", "reset", "visualize",
+    "replay", "log", "run", NULL))
+ die("can't use the builtin command '%s' as a term", term);
+
+ /* In theory, nothing prevents swapping
+ * completely good and bad, but this situation
+ * could be confusing and hasn't been tested
+ * enough. Forbid it for now.
+ */
+
+ 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 0;
+}
+
 int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
 {
- int next_all = 0;
+ int sub_command = 0;
  int no_checkout = 0;
+
+ enum sub_commands {
+ NEXT_ALL,
+ CHECK_TERM_FMT
+ };
+
  struct option options[] = {
- OPT_BOOL(0, "next-all", &next_all,
- N_("perform 'git bisect next'")),
+ OPT_CMDMODE(0, "next-all", &sub_command,
+ N_("perform 'git bisect next'"), NEXT_ALL),
  OPT_BOOL(0, "no-checkout", &no_checkout,
  N_("update BISECT_HEAD instead of checking out the current commit")),
+ OPT_CMDMODE(0, "check-term-format", &sub_command,
+ N_("check the format of the ref"), CHECK_TERM_FMT),
  OPT_END()
  };
 
  argc = parse_options(argc, argv, prefix, options,
      git_bisect_helper_usage, 0);
 
- if (!next_all)
+ if (sub_command == CHECK_TERM_FMT) {
+ if (argc == 2) {
+ if (argv[0] != NULL && argv[1] != NULL)
+ return check_term_format(argv[0], argv[1], 0);
+ else
+ die("no revision or term provided with check_for_term");
+ }
+ else
+ die("--check-term-format expects 2 arguments");
+ }
+
+ if (sub_command != NEXT_ALL && sub_command != CHECK_TERM_FMT)
  usage_with_options(git_bisect_helper_usage, options);
 
  /* next-all */
- return bisect_next_all(prefix, no_checkout);
+ if (sub_command == NEXT_ALL)
+ return bisect_next_all(prefix, no_checkout);
+
+ return 1;
 }
diff --git a/git-bisect.sh b/git-bisect.sh
index 5d1cb00..f63b83e 100755
--- a/git-bisect.sh
+++ b/git-bisect.sh
@@ -564,8 +564,8 @@ write_terms () {
  then
  die "$(gettext "please use two different terms")"
  fi
- check_term_format "$TERM_BAD" bad
- check_term_format "$TERM_GOOD" good
+ git bisect--helper --check-term-format "$TERM_BAD" bad
+ git bisect--helper --check-term-format "$TERM_GOOD" good
  printf '%s\n%s\n' "$TERM_BAD" "$TERM_GOOD" >"$GIT_DIR/BISECT_TERMS"
 }
 

--
https://github.com/git/git/pull/216
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

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

Johannes Schindelin
In reply to this post by pranitbauva1997
Hi Pranit,

On Tue, 22 Mar 2016, Pranit Bauva wrote:

> I did run the tests. They produce the same results as they did before.
> To ease review I will next time include these the output of the tests
> in the commented section.
>
> t6002-rev-list-bisect.sh : http://paste.ubuntu.com/15473728/
> t6030-bisect-porcelain.sh : http://paste.ubuntu.com/15473734/
> t6041-bisect-submodule.sh : http://paste.ubuntu.com/15473743/
>
> Is there any other test I would need to run?

Oh, I just meant to point out that you need to make sure that the entire
test suite passes after your patch series (and ideally, after every patch,
that is at least what I frequently test before sending out patch series).

There is not really a need to mention that you ran the test suite. There
is only a need to run it ;-)

Ciao,
Johannes
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

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

Johannes Schindelin
In reply to this post by pranitbauva1997
Hi Pranit,

On Tue, 22 Mar 2016, Pranit Bauva wrote:

> On Tue, Mar 22, 2016 at 8:41 PM, Johannes Schindelin
> <[hidden email]> wrote:
> >
> > On Tue, 22 Mar 2016, Johannes Schindelin wrote:
> >
> >> On Tue, 22 Mar 2016, Pranit Bauva wrote:
> >>
> >> > +   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);
> >>
> >> These two can be combined. Actually, these *four* can easily be combined:
> >>
> >>       if ((one_of(term, "bad", "new", NULL) && strcmp(orig, "bad")) ||
> >>           (one_of(term, "good", "old", NULL) && strcmp(orig, "good")))
> >>               die("can't change the meaning of term '%s'", term);
> >
> > Completely forgot to mention: This conversion skipped the comment
> >
> >         # In theory, nothing prevents swapping
> >         # completely good and bad, but this situation
> >         # could be confusing and hasn't been tested
> >         # enough. Forbid it for now.
> >
> > Let's port that comment over, too?
>
> Sure! Adding a comment won't harm anyone. We can remove it when its
> thoroughly tested.

I am actually not so eager to remove the comment...

Ciao,
Johannes
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

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

Johannes Schindelin
In reply to this post by Stefan Beller-4
Hi,

On Tue, 22 Mar 2016, Stefan Beller wrote:

> On Tue, Mar 22, 2016 at 10:52 AM, Pranit Bauva <[hidden email]> wrote:
> > OPT_CMDMODE() is actually a better option. I also noticed that it
> > isn't mentioned in Documentation/technical/api-parse-options.txt .
> > Should I send a patch to include it there just to make it easier for
> > someone who is new and isn't aware of the changes ?
>
> Patches to outdated documentation are most awesome. ;)

Yes!

Thanks,
Johannes
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

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

Johannes Schindelin
In reply to this post by pranitbauva1997
Hi Pranit,

On Wed, 23 Mar 2016, Pranit Bauva wrote:

> Convert the code literally without changing its design even though it
> seems that it is 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.

I still believe that it would make for an improvement to replace
"revision" by "orig_term".

> The argument handling is kind of hard coded right now because it is not
> really be meant to be used like this and this is just for testing
> purposes whether this new method is as functional as its counter part.
> The shell counter part of the method has been retained for historical
> purposes.

Well, maybe the argument handling is really meant to be used like this in
the end? Just skip that paragraph.

> Also using OPT_CMDMODE() to handle check-term-format and next-all
> because these sub commands are independent and error should be shown if
> used together and should be handled independently.

As is often the case, after some discussion a single patch becomes more
than just one change. This is what we see here, too: OPT_CMDMODE() is a
change that needs preparation of the existing code in
builtin/bisect--helper.c, and I would highly suggest to split that change
out into its own patch. It makes for a nicer story, and it is *much*
easier to review.

> This commit reduces the number of failed tests in
> t6030-bisect-porcelain.sh and t6041-bisect-submodule.sh

Oh? Which tests are supposed to fail? I do not see any failing tests
here...

> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
> index 3324229..ab3891c 100644
> --- a/builtin/bisect--helper.c
> +++ b/builtin/bisect--helper.c
> [...]
> +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 (one_of(term, "help", "start", "skip", "next", "reset", "visualize",
> +    "replay", "log", "run", NULL))

If I understood Junio correctly, he meant to line up the second line with
the corresponding level. In this case, as "replay" is a parameter of the
one_of() function, the indentation would look like this instead:

        if (one_of(term, "help", "start", "skip", "next", "reset", "visualize",
                   "replay", "log", "run", NULL))

> + die("can't use the builtin command '%s' as a term", term);
> +
> + /* In theory, nothing prevents swapping
> + * completely good and bad, but this situation
> + * could be confusing and hasn't been tested
> + * enough. Forbid it for now.
> + */

I see quite a few comments that put the closing "*/" on its own line, but
do not award the same pleasure to the opening "/*". Personally, I much
prefer the opening "/*" to have an entire line to itself, too, but I guess
that there is enough precendence in Git's source code to accept both
forms.

> + 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);

I am still convinced that

        if ((one_of(term, "bad", "new", NULL) && strcmp(orig_term, "bad")) ||
            (one_of(term, "good", "old", NULL) && strcmp(orig_term, "good")))
                die("can't change the meaning of term '%s'", term);

looks so much nicer.

>  int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
>  {
> - int next_all = 0;
> + int sub_command = 0;
>   int no_checkout = 0;
> +
> + enum sub_commands {
> + NEXT_ALL,
> + CHECK_TERM_FMT
> + };

Interesting. I did not think that Git's source code declares enums inside
functions, but builtin/remote.c's config_read_branches() does, so this
code is fine.

Still, the patch would be easier to review (corollary: bugs would have a
harder time to hide) if the change from OPT_BOOL to OPT_CMDMODE was done
in a separate, preparatory patch.

>   argc = parse_options(argc, argv, prefix, options,
>       git_bisect_helper_usage, 0);
>  
> - if (!next_all)
> + if (sub_command == CHECK_TERM_FMT) {
> + if (argc == 2) {
> + if (argv[0] != NULL && argv[1] != NULL)
> + return check_term_format(argv[0], argv[1], 0);
> + else
> + die("no revision or term provided with check_for_term");
> + }
> + else
> + die("--check-term-format expects 2 arguments");
> + }
> +
> + if (sub_command != NEXT_ALL && sub_command != CHECK_TERM_FMT)
>   usage_with_options(git_bisect_helper_usage, options);

Personally, I would prefer

- the usage_with_options() code to come before any sub_command processing,

- the sub_command enum to be initialized with -1, or alternatively
  hardcoding NEXT_ALL to 1,

- to avoid else clauses after an if () clause whose block returns or
  die()s,

- to order the clauses of an if/else in ascending size, i.e.

        if (argc != 2)
                die(...);
        if ...

- to avoid checking argv[i] for NULL when i < argc (it is the contract
  that argv[0..argc-1] are all non-NULL, so checking for it is unnecessary
  churn),

- use a switch() on sub_command instead of unrolled if () statements, and

- wrap the code at 80 columns/row (interpreting tabs as "up to 8 spaces").

The rest of the patch looks good.

Ciao,
Johannes
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

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

pranitbauva1997
On Wed, Mar 23, 2016 at 5:27 PM, Johannes Schindelin
<[hidden email]> wrote:

> Hi Pranit,
>
> On Wed, 23 Mar 2016, Pranit Bauva wrote:
>
>> Convert the code literally without changing its design even though it
>> seems that it is 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.
>
> I still believe that it would make for an improvement to replace
> "revision" by "orig_term".

I missed this. Will do it.

>> The argument handling is kind of hard coded right now because it is not
>> really be meant to be used like this and this is just for testing
>> purposes whether this new method is as functional as its counter part.
>> The shell counter part of the method has been retained for historical
>> purposes.
>
> Well, maybe the argument handling is really meant to be used like this in
> the end? Just skip that paragraph.

Sure.

>> Also using OPT_CMDMODE() to handle check-term-format and next-all
>> because these sub commands are independent and error should be shown if
>> used together and should be handled independently.
>
> As is often the case, after some discussion a single patch becomes more
> than just one change. This is what we see here, too: OPT_CMDMODE() is a
> change that needs preparation of the existing code in
> builtin/bisect--helper.c, and I would highly suggest to split that change
> out into its own patch. It makes for a nicer story, and it is *much*
> easier to review.
>
>> This commit reduces the number of failed tests in
>> t6030-bisect-porcelain.sh and t6041-bisect-submodule.sh
>
> Oh? Which tests are supposed to fail? I do not see any failing tests
> here...

What I meant by this is that before there were 55 out of 70 tests
which failed. After this patch, there are 3 tests out of 70 which
failed.

>> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
>> index 3324229..ab3891c 100644
>> --- a/builtin/bisect--helper.c
>> +++ b/builtin/bisect--helper.c
>> [...]
>> +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 (one_of(term, "help", "start", "skip", "next", "reset", "visualize",
>> +         "replay", "log", "run", NULL))
>
> If I understood Junio correctly, he meant to line up the second line with
> the corresponding level. In this case, as "replay" is a parameter of the
> one_of() function, the indentation would look like this instead:

I misunderstood his point.

>         if (one_of(term, "help", "start", "skip", "next", "reset", "visualize",
>                    "replay", "log", "run", NULL))
>
>> +             die("can't use the builtin command '%s' as a term", term);
>> +
>> +     /* In theory, nothing prevents swapping
>> +      * completely good and bad, but this situation
>> +      * could be confusing and hasn't been tested
>> +      * enough. Forbid it for now.
>> +      */
>
> I see quite a few comments that put the closing "*/" on its own line, but
> do not award the same pleasure to the opening "/*". Personally, I much
> prefer the opening "/*" to have an entire line to itself, too, but I guess
> that there is enough precendence in Git's source code to accept both
> forms.
>
>> +     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);
>
> I am still convinced that
>
>         if ((one_of(term, "bad", "new", NULL) && strcmp(orig_term, "bad")) ||
>             (one_of(term, "good", "old", NULL) && strcmp(orig_term, "good")))
>                 die("can't change the meaning of term '%s'", term);
>
> looks so much nicer.

True. I missed this point.

>>  int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
>>  {
>> -     int next_all = 0;
>> +     int sub_command = 0;
>>       int no_checkout = 0;
>> +
>> +     enum sub_commands {
>> +             NEXT_ALL,
>> +             CHECK_TERM_FMT
>> +     };
>
> Interesting. I did not think that Git's source code declares enums inside
> functions, but builtin/remote.c's config_read_branches() does, so this
> code is fine.

I didn't notice this before. Since git has the convention of declaring
enums outside function, it will be better to follow the trend rather
than allowing another trend to spread.

> Still, the patch would be easier to review (corollary: bugs would have a
> harder time to hide) if the change from OPT_BOOL to OPT_CMDMODE was done
> in a separate, preparatory patch.

I was confused about this. Now that u mention it, I will make the change.

>>       argc = parse_options(argc, argv, prefix, options,
>>                            git_bisect_helper_usage, 0);
>>
>> -     if (!next_all)
>> +     if (sub_command == CHECK_TERM_FMT) {
>> +             if (argc == 2) {
>> +                     if (argv[0] != NULL && argv[1] != NULL)
>> +                             return check_term_format(argv[0], argv[1], 0);
>> +                     else
>> +                             die("no revision or term provided with check_for_term");
>> +             }
>> +             else
>> +                     die("--check-term-format expects 2 arguments");
>> +     }
>> +
>> +     if (sub_command != NEXT_ALL && sub_command != CHECK_TERM_FMT)
>>               usage_with_options(git_bisect_helper_usage, options);
>
> Personally, I would prefer
>
> - the usage_with_options() code to come before any sub_command processing,
>
> - the sub_command enum to be initialized with -1, or alternatively
>   hardcoding NEXT_ALL to 1,

Oh! I now understand that initializing with 0 can be problematic.

> - to avoid else clauses after an if () clause whose block returns or
>   die()s,

Sure

> - to order the clauses of an if/else in ascending size, i.e.
>
>         if (argc != 2)
>                 die(...);
>         if ...
>
> - to avoid checking argv[i] for NULL when i < argc (it is the contract
>   that argv[0..argc-1] are all non-NULL, so checking for it is unnecessary
>   churn),

I wasn't aware of this.

> - use a switch() on sub_command instead of unrolled if () statements, and
I just browsed through some other parts and found that subcommands are
put in switch case

> - wrap the code at 80 columns/row (interpreting tabs as "up to 8 spaces").


> The rest of the patch looks good.
>
> Ciao,
> Johannes

Thanks!
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
1234 ... 6